-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(safer-cluster): add create_service_account variable #2138
feat(safer-cluster): add create_service_account variable #2138
Conversation
/gcbrun |
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.
Thanks for the contribution @jlubawy!
This might be considered a breaking change, regardless can you please add an upgrade similar to the examples at to example how/when this should be used: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/docs/upgrading_to_v33.0.md
/gcbrun |
aa1dc2d
to
4bbf60f
Compare
Hi @apeabody, I added a docs/upgrading_to_v34.0.md describing the change. For what it's worth I think it is backwards compatible the way I wrote it. I also added some additional comments around the new variable to explain the reasoning and rebased again on |
/gcbrun |
Thanks @jlubawy! Agreed, it should be backwards compatible for existing users. The doc will help explain when users may want to use it. |
/gcbrun |
/gcbrun |
Adds a
var.create_service_account
to thesafer-cluster
modules that when explicitly set tofalse
avoids the following error:This seems to happen if
var.compute_engine_service_account
is passed in, and the service account is being created at the same time (so the name/email is not computed yet):By explicitly passing a
var.create_service_account = false
it short circuits the calculations dependent onvar.service_account_name
:Happy to make any changes to help get this merged, it worked for my use-case. This also seemed like the easiest change to make while keeping backwards compatibility, but I'm open to other approaches.