-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Update resource_container_cluster.go.erb #11562
Conversation
Fix autopilot diff suppression func. We only need a diff if a cluster already exists.
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @slevenick, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
@slevenick This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
Hi Team, Can you take a look at this Request? It is one of the core requirement for the enterprise customer cc - @maci0 @slevenick |
@GoogleCloudPlatform/terraform-team @slevenick This PR has been waiting for review for 1 week. Please take a look! Use the label |
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.
We've had some discussions about this and I'm not sure this is the correct fix.
For one, this function is being used in 6 places across this file. Are we sure that we should be adding those fields to the initial plan for autopilot clusters? Is specifying things like disk size valid for autopilot clusters?
Second, if we're giving users the ability to set these fields on create, why would they not be able to update them? If a user can set additive_vpc_scope_dns_domain
on an autopilot cluster, if they modify that value it should show a diff.
If we decide to move forward with this change we will also need tests for setting the fields on an autopilot cluster on create
This I am not 100% sure about and would need to be verified/tested.
Autopilot doesn't support changing this block after the initial cluster creation. Standard GKE does though. So effectively it's immutable for Autopilot only, if this will change in the future I don't know but that is what autopilot currently supports. |
@slevenick This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
Ok, if it's immutable for Autopilot then we should probably remove the diff suppress func entirely for this field and instead add a customize diff that will trigger replacement if this field in particular changes and autopilot is set to true. You can see an example of similar behavior on Also, I don't think we want to change behavior on the fields that are sharing this diff suppress func unless we know their behavior. |
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.
See most recent comment
@maci0, this PR is waiting for action from you. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. If no action is taken, this PR will be closed in 28 days. This notification can be disabled with the |
close in favor of #11744 |
Fix autopilot diff suppression func.
We only need a diff if a cluster already exists.
This will fix terraform-google-modules/terraform-google-kubernetes-engine#2042 and other similar issues
Release Note Template for Downstream PRs (will be copied)