magic-modules icon indicating copy to clipboard operation
magic-modules copied to clipboard

Update resource_container_cluster.go.erb

Open maci0 opened this issue 1 year ago • 1 comments

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

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 

maci0 avatar Aug 28 '24 06:08 maci0

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.

github-actions[bot] avatar Aug 28 '24 06:08 github-actions[bot]

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

github-actions[bot] avatar Aug 30 '24 09:08 github-actions[bot]

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

cc - @maci0 @slevenick

ajinkya101 avatar Sep 01 '24 04:09 ajinkya101

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

github-actions[bot] avatar Sep 03 '24 09:09 github-actions[bot]

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.

maci0 avatar Sep 04 '24 21:09 maci0

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

github-actions[bot] avatar Sep 09 '24 09:09 github-actions[bot]

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.

slevenick avatar Sep 09 '24 13:09 slevenick

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

github-actions[bot] avatar Sep 23 '24 09:09 github-actions[bot]

close in favor of https://github.com/GoogleCloudPlatform/magic-modules/pull/11744

maci0 avatar Sep 24 '24 08:09 maci0