-
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
Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set #10101
Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set #10101
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @SarahFrench, 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. |
07731db
to
4de9c66
Compare
4de9c66
to
8ce0dbd
Compare
@SarahFrench a kindly reminder |
Thanks! This had fallen off my radar |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
/gcbrun |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
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.
Hi @Francis-Liu - the code changes make sense to me, but I'm wondering about what testing can be done to verify this change.
Am I right that prior to this PR's changes a user would provision a private flexible cluster with master_ipv4_cidr_block
set and on the next apply Terraform would suggest to replace the whole cluster due to the detected diff?
Would you be comfortable adding a new acceptance test or updating an existing one so that it fails prior to this change (due to a non-empty plan) and passes afterwards? By default the test framework will not expect a non empty plan and will fail after provisioning something, which suggests we might not have a test that provisions a cluster configured in the way that triggers this problem.
Yes. Precisely, the diff is in |
Let me try to add a acceptance test case as you suggested, to capture potential failures after we make the further API change. |
Thanks, that'd be useful to have! I asked in the provider team for advice about how to time this release with the API update. Next week I'm at a conference so won't be able to review things properly until March 25th onwards. If this PR needs to be reviewed and merged earlier, to enable release on a specific date please let us know. |
Following on from above, handing off to our on-call for the next week: @roaks3. Ryan, if this PR doesn't need anything before the 25th please feel free to reassign back to me |
@roaks3 I have added the acceptance test. Could you please approve & merge this PR so it goes to release |
/gcbrun |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerCluster_withCidrBlockWithoutPrivateEndpointSubnetwork |
|
7ee8e93
to
2b7dcb8
Compare
/gcbrun |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerCluster_withCidrBlockWithoutPrivateEndpointSubnetwork |
|
…4_cidr_block is set Signed-off-by: Francis Liu <[email protected]>
2b7dcb8
to
49f94b9
Compare
/gcbrun |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerCluster_withCidrBlockWithoutPrivateEndpointSubnetwork |
|
…et (GoogleCloudPlatform#10101) * Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set * acceptance test: Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set Signed-off-by: Francis Liu <[email protected]> --------- Signed-off-by: Francis Liu <[email protected]>
…et (GoogleCloudPlatform#10101) * Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set * acceptance test: Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set Signed-off-by: Francis Liu <[email protected]> --------- Signed-off-by: Francis Liu <[email protected]>
…et (GoogleCloudPlatform#10101) * Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set * acceptance test: Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set Signed-off-by: Francis Liu <[email protected]> --------- Signed-off-by: Francis Liu <[email protected]>
…et (GoogleCloudPlatform#10101) * Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set * acceptance test: Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set Signed-off-by: Francis Liu <[email protected]> --------- Signed-off-by: Francis Liu <[email protected]>
…et (GoogleCloudPlatform#10101) * Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set * acceptance test: Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set Signed-off-by: Francis Liu <[email protected]> --------- Signed-off-by: Francis Liu <[email protected]>
…et (GoogleCloudPlatform#10101) * Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set * acceptance test: Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set Signed-off-by: Francis Liu <[email protected]> --------- Signed-off-by: Francis Liu <[email protected]>
…et (GoogleCloudPlatform#10101) * Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set * acceptance test: Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set Signed-off-by: Francis Liu <[email protected]> --------- Signed-off-by: Francis Liu <[email protected]>
…et (GoogleCloudPlatform#10101) * Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set * acceptance test: Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set Signed-off-by: Francis Liu <[email protected]> --------- Signed-off-by: Francis Liu <[email protected]>
…et (GoogleCloudPlatform#10101) * Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set * acceptance test: Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set Signed-off-by: Francis Liu <[email protected]> --------- Signed-off-by: Francis Liu <[email protected]>
…et (GoogleCloudPlatform#10101) * Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set * acceptance test: Suppress private_endpoint_subnetwork when master_ipv4_cidr_block is set Signed-off-by: Francis Liu <[email protected]> --------- Signed-off-by: Francis Liu <[email protected]>
For private flexible clusters, user are still allowed to set
master_ipv4_cidr_block
. In this situation,private_endpoint_subnetwork
will return a non-empty subnet name, which doesn't match the empty subnet in Terraform config (user can set either cidr block or subnet, or neither, but not both, which is error-checked by API). We should ignore the subnet diff in this case. This PR is intended to fix the same issue #10089 attempted to fix. #10089 was reverted by #10096.Release Note Template for Downstream PRs (will be copied)