-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
9e53524
to
51bf0c9
Compare
@rileykarson I have rebased the changes and the diff is now correct. |
NOTES ON EXAMPLE CHANGES:
|
description = "The name of the service account to use for Tiller." | ||
} | ||
|
||
variable "tls_subject" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide defaults like we do in other modules now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Done.
7310890
to
0eb84e6
Compare
3f966fb
to
3203b81
Compare
UPDATE: Rebased on master to resolve merge conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! Sorry for the delay in reviewing... The review session was (again) very helpful.
// os.Setenv("SKIP_create_terratest_options", "true") | ||
// os.Setenv("SKIP_terraform_apply", "true") | ||
// os.Setenv("SKIP_validate", "true") | ||
// os.Setenv("SKIP_cleanup", "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add // os.Setenv("SKIP_validate_upgrade", "true")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
}) | ||
|
||
test_structure.RunTestStage(t, "validate_upgrade", func() { | ||
// Make sure the upgrade command mentioned in the docs actually works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-Authored-By: autero1 <[email protected]>
Thanks for the review folks! I'm going to merge and release this now. |
This implements modules to do TLS management in pure terraform using the
kubernetes
andtls
providers, as an alternative to usingkubergrunt
. In this model, the user does not need to installkubergrunt
at all to use all the features. As a part of this, this updates the root example to use the pure terraform version, while migrating thekubergrunt
version to the examples folder.Note that the major drawback of this approach is that the TLS cert information will leak into the Terraform state file.
This is understandably a big PR and a big change. We do NOT have to merge this right away with launch so close.