Add allow_net_admin field to google_container_cluster resource #8323

jeperetz · 2023-07-12T00:29:13Z

Fixes issue hashicorp/terraform-provider-google#15136

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read Write release notes before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

container: added `allow_net_admin` field to `google_container_cluster` resource

…test

modular-magician · 2023-07-12T00:29:49Z

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @NickElliot, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

Your assigned reviewer will help review your code by:

Ensuring it's backwards compatible, covers common error cases, etc.
Summarizing the change into a user-facing changelog note.
Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

modular-magician · 2023-07-12T00:43:36Z

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

Diff report

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

Terraform GA: Diff ( 2 files changed, 80 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 2 files changed, 80 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_container_cluster (217 total tests)
Untested fields: allow_net_admin

Please add acceptance tests which include these fields.

modular-magician · 2023-07-12T02:26:11Z

Tests analytics

Total tests: 2853
Passed tests 2554
Skipped tests: 296
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccContainerCluster_autopilot_net_admin|TestAccComputeFirewallPolicyRule_multipleRules

Get to know how VCR tests work

modular-magician · 2023-07-12T02:40:01Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerCluster_autopilot_net_admin[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:}}$
TestAccContainerAwsNodePool_BetaBasicHandWritten[Error message] [Debug log]
TestAccComputeFirewallPolicyRule_multipleRules[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

NickElliot

At initial pass looks good. Could you also update the documentation to include the new field?

While that update is made I'm going to verify that the one-way required_with doesn't produce undesired forced recreations as it appears to be an unprecedented configuration. It may end up being better to remove that terraform side validation just to be sure, because as the allow_net_admin field is part of the Autopilot block, I assume that the API will reject a request that supplies only allow_net_admin, or it will simply do nothing if included on its own.

modular-magician · 2023-07-19T18:54:16Z

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

Diff report

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

Terraform GA: Diff ( 3 files changed, 83 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 3 files changed, 83 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_container_cluster (217 total tests)
Untested fields: allow_net_admin

Please add acceptance tests which include these fields.

modular-magician · 2023-07-19T20:44:54Z

Tests analytics

Total tests: 2872
Passed tests 2570
Skipped tests: 299
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeInstance_minCpuPlatform|TestAccComputeFirewallPolicyRule_multipleRules

Get to know how VCR tests work

modular-magician · 2023-07-19T20:50:01Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeInstance_minCpuPlatform[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:}}$
TestAccContainerAwsNodePool_BetaBasicHandWritten[Error message] [Debug log]
TestAccComputeFirewallPolicyRule_multipleRules[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

NickElliot

LGTM! to @hao-nan-li for final approval

hao-nan-li · 2023-07-19T23:32:03Z

mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb

@@ -2750,6 +2769,29 @@ func resourceContainerClusterUpdate(d *schema.ResourceData, meta interface{}) er
 		log.Printf("[INFO] GKE cluster %s's cluster-wide autoscaling has been updated", d.Id())
 	}

+	if d.HasChange("allow_net_admin") {


Is this field updatable? If so can you add an update test?

Yes the field is updatable. I added update steps to the test to toggle the feature off and on again.

modular-magician · 2023-07-21T01:55:32Z

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

Diff report

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

Terraform GA: Diff ( 3 files changed, 97 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 3 files changed, 97 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_container_cluster (217 total tests)
Untested fields: allow_net_admin

Please add acceptance tests which include these fields.

modular-magician · 2023-07-21T03:39:22Z

Tests analytics

Total tests: 2875
Passed tests 2573
Skipped tests: 299
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerCluster_autopilot_net_admin|TestAccComputeFirewallPolicyRule_multipleRules|TestAccContainerAwsNodePool_BetaBasicHandWritten

Get to know how VCR tests work

modular-magician · 2023-07-21T04:53:40Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerCluster_autopilot_net_admin[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:}}$
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]
TestAccContainerAwsNodePool_BetaBasicHandWritten[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

hao-nan-li · 2023-07-21T17:02:49Z

/gcbrun

modular-magician · 2023-07-21T17:18:35Z

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

Diff report

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

Terraform GA: Diff ( 3 files changed, 97 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 3 files changed, 97 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_container_cluster (217 total tests)
Untested fields: allow_net_admin

Please add acceptance tests which include these fields.

modular-magician · 2023-07-21T19:04:47Z

Tests analytics

Total tests: 2875
Passed tests 2574
Skipped tests: 299
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccComputeFirewallPolicyRule_multipleRules|TestAccContainerAwsNodePool_BetaBasicHandWritten

Get to know how VCR tests work

modular-magician · 2023-07-21T19:07:23Z

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]
TestAccContainerAwsNodePool_BetaBasicHandWritten[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

hao-nan-li · 2023-07-24T17:43:22Z

/gcbrun

modular-magician · 2023-07-24T17:56:48Z

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

Diff report

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

Terraform GA: Diff ( 3 files changed, 97 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 3 files changed, 97 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_container_cluster (217 total tests)
Untested fields: allow_net_admin

Please add acceptance tests which include these fields.

modular-magician · 2023-07-24T19:13:09Z

Tests analytics

Total tests: 2875
Passed tests 2574
Skipped tests: 299
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules

Get to know how VCR tests work

modular-magician · 2023-07-24T19:22:47Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeFirewallPolicyRule_multipleRules[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:}}$
TestAccContainerAwsNodePool_BetaBasicHandWritten[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

…eCloudPlatform#8323) * add net_admin field to container cluster for autopilot clusters * add allow_net_admin tests * tweak param name * fix net_admin test format string * fix syntax bug * fix api field typo * version safety and ImportStateVerifyIgnore for min_master_version in test * comment change * Add documentation and remove provider-side validation * add update test

jeperetz added 8 commits July 10, 2023 13:59

add net_admin field to container cluster for autopilot clusters

5b147ff

add allow_net_admin tests

ebb62de

tweak param name

f799ff3

fix net_admin test format string

94cdd6a

fix syntax bug

1f5ec62

fix api field typo

5dc28c0

version safety and ImportStateVerifyIgnore for min_master_version in …

7a0c37b

…test

comment change

6c46273

modular-magician requested a review from NickElliot July 12, 2023 00:29

NickElliot requested changes Jul 12, 2023

View reviewed changes

Add documentation and remove provider-side validation

d9717ec

modular-magician requested a review from NickElliot July 19, 2023 18:38

NickElliot reviewed Jul 19, 2023

View reviewed changes

NickElliot requested a review from hao-nan-li July 19, 2023 21:32

hao-nan-li reviewed Jul 19, 2023

View reviewed changes

add update test

2a7afab

modular-magician requested review from NickElliot and hao-nan-li July 21, 2023 01:41

hao-nan-li approved these changes Jul 24, 2023

View reviewed changes

NickElliot approved these changes Jul 24, 2023

View reviewed changes

hao-nan-li merged commit 866f90f into GoogleCloudPlatform:main Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add allow_net_admin field to google_container_cluster resource #8323

Add allow_net_admin field to google_container_cluster resource #8323

jeperetz commented Jul 12, 2023 •

edited by NickElliot

Loading

modular-magician commented Jul 12, 2023

modular-magician commented Jul 12, 2023

modular-magician commented Jul 12, 2023

modular-magician commented Jul 12, 2023

NickElliot left a comment

modular-magician commented Jul 19, 2023

modular-magician commented Jul 19, 2023

modular-magician commented Jul 19, 2023

NickElliot left a comment

hao-nan-li Jul 19, 2023

jeperetz Jul 21, 2023

modular-magician commented Jul 21, 2023

modular-magician commented Jul 21, 2023

modular-magician commented Jul 21, 2023

hao-nan-li commented Jul 21, 2023

modular-magician commented Jul 21, 2023

modular-magician commented Jul 21, 2023

modular-magician commented Jul 21, 2023

hao-nan-li commented Jul 24, 2023

modular-magician commented Jul 24, 2023

modular-magician commented Jul 24, 2023

modular-magician commented Jul 24, 2023

Add allow_net_admin field to google_container_cluster resource #8323

Add allow_net_admin field to google_container_cluster resource #8323

Conversation

jeperetz commented Jul 12, 2023 • edited by NickElliot Loading

modular-magician commented Jul 12, 2023

modular-magician commented Jul 12, 2023

Diff report

Missing test report

modular-magician commented Jul 12, 2023

Tests analytics

Action taken

modular-magician commented Jul 12, 2023

Rerun these tests in REPLAYING mode to catch issues

NickElliot left a comment

Choose a reason for hiding this comment

modular-magician commented Jul 19, 2023

Diff report

Missing test report

modular-magician commented Jul 19, 2023

Tests analytics

Action taken

modular-magician commented Jul 19, 2023

Rerun these tests in REPLAYING mode to catch issues

NickElliot left a comment

Choose a reason for hiding this comment

hao-nan-li Jul 19, 2023

Choose a reason for hiding this comment

jeperetz Jul 21, 2023

Choose a reason for hiding this comment

modular-magician commented Jul 21, 2023

Diff report

Missing test report

modular-magician commented Jul 21, 2023

Tests analytics

Action taken

modular-magician commented Jul 21, 2023

Rerun these tests in REPLAYING mode to catch issues

hao-nan-li commented Jul 21, 2023

modular-magician commented Jul 21, 2023

Diff report

Missing test report

modular-magician commented Jul 21, 2023

Tests analytics

Action taken

modular-magician commented Jul 21, 2023

hao-nan-li commented Jul 24, 2023

modular-magician commented Jul 24, 2023

Diff report

Missing test report

modular-magician commented Jul 24, 2023

Tests analytics

Action taken

modular-magician commented Jul 24, 2023

Rerun these tests in REPLAYING mode to catch issues

jeperetz commented Jul 12, 2023 •

edited by NickElliot

Loading