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

Breaking change: Rework taint model in GKE #9011

Conversation

rileykarson
Copy link
Member

@rileykarson rileykarson commented Sep 19, 2023

Part of hashicorp/terraform-provider-google#7928, hashicorp/terraform-provider-google#13309

This applies roughly the same model we're using with labels, where we limit what we write back into state during reads based on what's already in state. This gives us the opportunity to drastically simplify the interactions of the field, while unfortunately trading away authoritative-ness. We could easily reintroduce an authoritative field by making effective_taints O+C though- one of the benefits of effective_X output fields here and in the labels rework.

Not having update support is awkward- we'd be able to pick up new keys to manage if so, and delete keys, but that'll only update google_container_node_pool when added due to other complications with GKE cluster.

TestAccContainerNodePool_withSandboxConfig gets drastically simpler with this change- it's boring now! There's an unmanaged taint being added in the background there, with the withNodeConfig tests already covering taints added at create time.

Release Note Template for Downstream PRs (will be copied)

container: reworked the `taint` field in `google_container_cluster` and `google_container_node_pool` to only manage a subset of taint keys based on those already in state. Most existing resources are unaffected, unless they use `sandbox_config`- see upgrade guide for details.
container: added the `effective_taints` attribute to `google_container_cluster` and `google_container_node_pool`, outputting all known taint values

@rileykarson rileykarson changed the title Rework taint model in GKE Breaking change: Rework taint model in GKE Sep 19, 2023
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

  • Field node_config.taint transitioned from optional+computed to optional google_container_cluster - reference
  • Field node_config.taint transitioned from optional+computed to optional google_container_node_pool - reference
  • Field node_pool.node_config.taint transitioned from optional+computed to optional google_container_cluster - reference

If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An override-breaking-change label can be added to allow merging.

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 81 insertions(+), 21 deletions(-))
Terraform Beta: Diff ( 5 files changed, 82 insertions(+), 174 deletions(-))

@rileykarson rileykarson added the override-breaking-change Allows a potential breaking change to be merged label Sep 19, 2023
@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3035
Passed tests 2725
Skipped tests: 304
Affected tests: 6

Action taken

Found 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccContainerNodePool_withSandboxConfig|TestAccContainerNodePool_withNodeConfig|TestAccContainerCluster_withSandboxConfig|TestAccContainerCluster_withNodeConfig|TestAccDataprocClusterIamPolicy|TestAccVertexAIIndexEndpoint_updated

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerNodePool_withSandboxConfig[Debug log]
TestAccDataprocClusterIamPolicy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerNodePool_withNodeConfig[Error message] [Debug log]
TestAccContainerCluster_withSandboxConfig[Error message] [Debug log]
TestAccContainerCluster_withNodeConfig[Error message] [Debug log]
TestAccVertexAIIndexEndpoint_updated[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

@c2thorn
Copy link
Member

c2thorn commented Sep 20, 2023

TestAccVertexAIIndexEndpoint_updated is unrelated. The remaining tests likely need to be ignored on import.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

  • Field node_config.taint transitioned from optional+computed to optional google_container_cluster - reference
  • Field node_config.taint transitioned from optional+computed to optional google_container_node_pool - reference
  • Field node_pool.node_config.taint transitioned from optional+computed to optional google_container_cluster - reference

If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An override-breaking-change label can be added to allow merging.

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 6 files changed, 91 insertions(+), 29 deletions(-))
Terraform Beta: Diff ( 6 files changed, 93 insertions(+), 183 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3068
Passed tests 2755
Skipped tests: 307
Affected tests: 6

Action taken

Found 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange|TestAccBigQueryDataTable_bigtable|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccVertexAIIndexEndpoint_updated

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange[Error message] [Debug log]
TestAccBigQueryDataTable_bigtable[Error message] [Debug log]
TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample[Error message] [Debug log]
TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample[Error message] [Debug log]
TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample[Error message] [Debug log]
TestAccVertexAIIndexEndpoint_updated[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

@rileykarson rileykarson merged commit c896b46 into GoogleCloudPlatform:FEATURE-BRANCH-major-release-5.0.0 Sep 22, 2023
10 of 11 checks passed
@rileykarson rileykarson deleted the gke-node-pool-taint branch September 22, 2023 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
override-breaking-change Allows a potential breaking change to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants