-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
google container_cluster master_auth should be optional #14630
google container_cluster master_auth should be optional #14630
Conversation
f781ede
to
3a929e9
Compare
3a929e9
to
8a4576f
Compare
MasterAuth: &container.MasterAuth{ | ||
if v, ok := d.GetOk("master_auth"); ok { | ||
masterAuths := v.([]interface{}) | ||
if len(masterAuths) > 1 { |
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.
While you're here, would you mind removing this check and instead putting MaxItems: 1 in the schema defined above? That'll move the check from the apply phase to the plan phase.
ForceNew: true, | ||
MaxItems: 1, |
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.
Whoops, missed this earlier- you'll also need to set this to computed to get tests without master_auth to pass, since the server does return master_auth info.
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.
Why do the tests pass 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.
Are you running the acceptance tests or are you just looking at the travis build output? The build passes but the acceptance tests didn't when I ran them (https://github.com/hashicorp/terraform/blob/master/.github/CONTRIBUTING.md#running-an-acceptance-test)
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.
Ohh right, forgot about that.
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, thanks @dwradcliffe!
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
I'm not sure why master_auth is required; it's not even an option when you create a cluster via the console today.
This should make it optional.