Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

maci0
Copy link

@maci0 maci0 commented Aug 28, 2024

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)

Fix autopilot diff suppression func.
We only need a diff if a cluster already exists.

This will fix https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/issues/2042 and other similar issues 

Fix autopilot diff suppression func.
We only need a diff if a cluster already exists.
Copy link

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.

@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Aug 28, 2024
Copy link

@slevenick This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

@ajinkya101
Copy link

Hi Team, Can you take a look at this Request? It is one of the core requirement for the enterprise customer

cc - @maci0 @slevenick

Copy link

github-actions bot commented Sep 3, 2024

@GoogleCloudPlatform/terraform-team @slevenick This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link
Contributor

@slevenick slevenick left a 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

@maci0
Copy link
Author

maci0 commented Sep 4, 2024

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?

This I am not 100% sure about and would need to be verified/tested.

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.

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.
I'm sure there's another more proper way to implement this edge case but personally I am not too familiar with the codebase here.

Copy link

github-actions bot commented Sep 9, 2024

@slevenick This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

@slevenick
Copy link
Contributor

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?

This I am not 100% sure about and would need to be verified/tested.

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.

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. I'm sure there's another more proper way to implement this edge case but personally I am not too familiar with the codebase here.

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 containerClusterAutopilotCustomizeDiff, and this could potentially be added to that function.

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.

Copy link
Contributor

@slevenick slevenick left a 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

Copy link

@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.

Image showing the re-request review button

If no action is taken, this PR will be closed in 28 days.

This notification can be disabled with the disable-automatic-closure label.

@maci0
Copy link
Author

maci0 commented Sep 24, 2024

close in favor of #11744

@maci0 maci0 closed this Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

additive_vpc_scope_dns_domain feature support for autopilot mode GKE Cluster
4 participants