Add Update Connector Support for VPC Access Connectors#8205
Conversation
|
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @SarahFrench, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
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. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
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 Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 5 files changed, 188 insertions(+), 9 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 13 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccVPCAccessConnector_vpcAccessConnectorMachineAndInstancesChanged|TestAccVPCAccessConnector_vpcAccessConnectorThroughput|TestAccVPCAccessConnector_vpcAccessConnectorSharedVpcExample|TestAccVPCAccessConnector_vpcAccessConnectorExample|TestAccComputeFirewallPolicyRule_multipleRules|TestAccComputeNetworkEndpoints_networkEndpointsBasic|TestAccVPCAccessConnectorDatasource_basic|TestAccCloudFunctionsFunction_vpcConnector|TestAccCloudRunV2Service_cloudrunv2ServiceFullUpdate|TestAccCloudRunV2Service_cloudrunv2ServiceVpcaccessExample|TestAccCloudRunV2Job_cloudrunv2JobFullUpdate|TestAccCloudRunV2Job_cloudrunv2JobVpcaccessExample|TestAccAppEngineStandardAppVersion_update |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
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 Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 5 files changed, 188 insertions(+), 9 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 13 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccVPCAccessConnectorDatasource_basic|TestAccComputeNetworkEndpoints_networkEndpointsBasic|TestAccCloudFunctionsFunction_vpcConnector|TestAccCloudRunV2Job_cloudrunv2JobVpcaccessExample|TestAccCloudRunV2Service_cloudrunv2ServiceFullUpdate|TestAccCloudRunV2Service_cloudrunv2ServiceVpcaccessExample|TestAccCloudRunV2Job_cloudrunv2JobFullUpdate|TestAccComputeFirewallPolicyRule_multipleRules|TestAccAppEngineStandardAppVersion_update|TestAccVPCAccessConnector_vpcAccessConnectorThroughput|TestAccVPCAccessConnector_vpcAccessConnectorMachineAndInstancesChanged|TestAccVPCAccessConnector_vpcAccessConnectorExample|TestAccVPCAccessConnector_vpcAccessConnectorSharedVpcExample |
I discussed with the Google terraform provider team and I believe this change should not be breaking. |
SarahFrench
left a comment
There was a problem hiding this comment.
Hi @jashansudan, thanks for your patience when waiting for my review 🙏 .
I can see via the acceptance test that the three mutable fields can be updated successfully. And thank you for checking about the suspected breaking changes in relation to default values.
Also, I confirmed that updates where the values of min_instances and max_instances are reduced results in a forced replacement of the resource. The testing framework doesn't expose an easy way to assert whether an update or force-replace action occurs, and workarounds usually need access to a "create_time" field (which this resource lacks). Because of this, I'm happy to merge this PR without a test that checks the behaviour introduced via resource_definition.
|
I'm going to re-run the acceptance tests in our CI, as I can see it timed out previously. The new test added in this PR passed when I ran it locally, so I don't expect any issues, but it would be good to have the test pass here before merging. /gcbrun |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
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 Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 5 files changed, 188 insertions(+), 9 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 14 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccVPCAccessConnector_vpcAccessConnectorMachineAndInstancesChanged|TestAccVPCAccessConnector_vpcAccessConnectorThroughput|TestAccVPCAccessConnector_vpcAccessConnectorExample|TestAccVPCAccessConnector_vpcAccessConnectorSharedVpcExample|TestAccVPCAccessConnectorDatasource_basic|TestAccProjectIamPolicy_invalidMembers|TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccCloudRunV2Job_cloudrunv2JobVpcaccessExample|TestAccCloudFunctionsFunction_vpcConnector|TestAccCloudRunV2Service_cloudrunv2ServiceFullUpdate|TestAccCloudRunV2Service_cloudrunv2ServiceVpcaccessExample|TestAccCloudRunV2Job_cloudrunv2JobFullUpdate|TestAccAppEngineStandardAppVersion_update|TestAccComputeFirewallPolicyRule_multipleRules |
|
Rerun these tests in REPLAYING mode to catch issues
|
There was a problem hiding this comment.
It's a good thing I re-ran the tests, as a couple of failing tests are linked to this PR. Are you able to access the error message and debug logs linked in this message?
The tests that appear linked to this PR's changes are:
- TestAccVPCAccessConnector_vpcAccessConnectorExample
- Error = "Operation failed: Invalid IP CIDR range was provided. It conflicts with an existing subnetwork. Please delete the connector manually."
- I'm not sure if it's something we can fix in this PR directly
- TestAccCloudFunctionsFunction_vpcConnector
- Error = "A Connector must specify either max_throughput or max_instances."
- edits to this file should address that failure
- TestAccAppEngineStandardAppVersion_update
- Error = "A Connector must specify either max_throughput or max_instances."
- edit to this file will address this
|
I've went ahead and hopefully fixed all the tests. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
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 Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 8 files changed, 194 insertions(+), 14 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 11 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccClouddeployTarget_Target|TestAccVPCAccessConnector_vpcAccessConnectorExample|TestAccComputeFirewallPolicyRule_multipleRules|TestAccClouddeployDeliveryPipeline_VerifyDeliveryPipeline|TestAccClouddeployDeliveryPipeline_DeliveryPipeline|TestAccClouddeployDeliveryPipeline_CanaryrunDeliveryPipeline|TestAccClouddeployDeliveryPipeline_CanaryServiceNetworkingDeliveryPipeline|TestAccClouddeployDeliveryPipeline_CanaryDeliveryPipeline|TestAccCloudFunctionsFunction_vpcConnector|TestAccAppEngineStandardAppVersion_update |
|
Rerun these tests in REPLAYING mode to catch issues
|
|
Looks like the tests are all fixed, thanks! The two tests that failed above are unrelated to this PR. I'm seeing an unexpected diff in the diff report - affecting the .teamcity/components/generated/services.kt file in the GA provider repo. That file is generated based on the service package directories present in the provider, and there were some new service packages created since this PR was started. Usually we'd ask for PRs to be rebased but in this case I believe that the unexpected diff won't actually happen after merging this PR. I want to test my understanding, so will approve and merge. Thanks for your patience! |
|
Ah, main branch is currently protected to help preparation of a patch release. I'll merge once it's possible. |
…atform#8205) * Add Update Connetor Support for VPC Access Connectors * fix lint * fix go lint * fix tests
…eCloudPlatform#8205)" This reverts commit 85ff1c2.
…atform#8205) * Add Update Connetor Support for VPC Access Connectors * fix lint * fix go lint * fix tests
…atform#8205) * Add Update Connetor Support for VPC Access Connectors * fix lint * fix go lint * fix tests
…atform#8205) * Add Update Connetor Support for VPC Access Connectors * fix lint * fix go lint * fix tests
Adds support for updating Serverless VPC Connectors.
If this PR is for Terraform, I acknowledge that I have:
make testandmake lintin the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)