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

Fix permadiff on google_vpc_access_connector resource with breaking changes (min_instances, max_instances, min_throughput, max_throughput fields)

Open SarahFrench opened this issue 10 months ago • 44 comments

Fixes https://github.com/hashicorp/terraform-provider-google/issues/15278

When reviewing this PR please see:

  • This comment for context about this PR : https://github.com/GoogleCloudPlatform/magic-modules/pull/10313#issuecomment-2159065620
  • This comment about the decision to make the two fields conflict in the schema: https://github.com/GoogleCloudPlatform/magic-modules/pull/10313#issuecomment-2159101760
  • Note to the reviewer when I marked this ready for review: https://github.com/GoogleCloudPlatform/magic-modules/pull/10313#issuecomment-2161335142

Release Note Template for Downstream PRs (will be copied)

vpcaccess: removed default values for `min_throughput` and `min_instances` fields on `google_vpc_access_connector` and made them default to values returned from the API when not provided by users
vpcaccess: added a conflicting fields restriction between `min_throughput` and `min_instances` fields on `google_vpc_access_connector`
vpcaccess: added a conflicting fields restriction between `max_throughput` and `max_instances` fields on `google_vpc_access_connector`

SarahFrench avatar Mar 28 '24 13:03 SarahFrench

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.

google provider: Diff ( 1 file changed, 52 insertions(+)) google-beta provider: Diff ( 1 file changed, 52 insertions(+))

modular-magician avatar Mar 28 '24 13:03 modular-magician

Tests analytics

Total tests: 5 Passed tests: 4 Skipped tests: 0 Affected tests: 1

Click here to see the affected service packages
  • vpcaccess

Action taken

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

Get to know how VCR tests work

modular-magician avatar Mar 28 '24 13:03 modular-magician

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$ TestAccVPCAccessConnector_vpcAccessConnectorThroughput_maxThroughput[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

modular-magician avatar Mar 28 '24 13:03 modular-magician

/gcbrun

SarahFrench avatar May 09 '24 20:05 SarahFrench

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.

google provider: Diff ( 1 file changed, 52 insertions(+)) google-beta provider: Diff ( 1 file changed, 52 insertions(+))

modular-magician avatar May 09 '24 20:05 modular-magician

Tests analytics

Total tests: 5 Passed tests: 4 Skipped tests: 0 Affected tests: 1

Click here to see the affected service packages
  • vpcaccess

Action taken

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

Get to know how VCR tests work

modular-magician avatar May 09 '24 20:05 modular-magician

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$ TestAccVPCAccessConnector_vpcAccessConnectorThroughput_maxThroughput[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

modular-magician avatar May 09 '24 20:05 modular-magician

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.

google provider: Diff ( 2 files changed, 52 insertions(+), 2 deletions(-)) google-beta provider: Diff ( 2 files changed, 52 insertions(+), 2 deletions(-))

Breaking Change(s) Detected

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

  • Field max_throughput default value changed from 300 to on google_vpc_access_connector - reference
  • Field min_throughput default value changed from 200 to on google_vpc_access_connector - 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.

Missing test report

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

Resource: google_vpc_access_connector (12 total tests) Please add an acceptance test which includes these fields. The test should include the following:

resource "google_vpc_access_connector" "primary" {
  min_throughput = # value needed
}


modular-magician avatar Jun 10 '24 14:06 modular-magician

Tests analytics

Total tests: 5 Passed tests: 0 Skipped tests: 0 Affected tests: 5

Click here to see the affected service packages
  • vpcaccess

Action taken

Found 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccVPCAccessConnectorDatasource_basic|TestAccVPCAccessConnector_vpcAccessConnectorExample|TestAccVPCAccessConnector_vpcAccessConnectorSharedVpcExample|TestAccVPCAccessConnector_vpcAccessConnectorThroughput|TestAccVPCAccessConnector_vpcAccessConnectorThroughput_maxThroughput

Get to know how VCR tests work

modular-magician avatar Jun 10 '24 14:06 modular-magician

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$ TestAccVPCAccessConnectorDatasource_basic[Error message] [Debug log] TestAccVPCAccessConnector_vpcAccessConnectorExample[Error message] [Debug log] TestAccVPCAccessConnector_vpcAccessConnectorSharedVpcExample[Error message] [Debug log] TestAccVPCAccessConnector_vpcAccessConnectorThroughput[Error message] [Debug log] TestAccVPCAccessConnector_vpcAccessConnectorThroughput_maxThroughput[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

modular-magician avatar Jun 10 '24 14:06 modular-magician

Just rebased on main because this PR's branch was pretty old.

Before the rebase this test failure showed how the permadiff occurs : https://github.com/GoogleCloudPlatform/magic-modules/pull/10313#issuecomment-2025212589

SarahFrench avatar Jun 10 '24 15:06 SarahFrench

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.

google provider: Diff ( 8 files changed, 72 insertions(+), 6 deletions(-)) google-beta provider: Diff ( 8 files changed, 72 insertions(+), 6 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 2 insertions(+)) Open in Cloud Shell: Diff ( 2 files changed, 4 insertions(+))

Breaking Change(s) Detected

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

  • Field max_throughput default value changed from 300 to on google_vpc_access_connector - reference
  • Field min_throughput default value changed from 200 to on google_vpc_access_connector - 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.

modular-magician avatar Jun 10 '24 15:06 modular-magician

Tests analytics

Total tests: 36 Passed tests: 31 Skipped tests: 1 Affected tests: 4

Click here to see the affected service packages
  • appengine
  • cloudfunctions
  • vpcaccess

Action taken

Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccVPCAccessConnector_vpcAccessConnectorExample|TestAccVPCAccessConnector_vpcAccessConnectorSharedVpcExample|TestAccVPCAccessConnector_vpcAccessConnectorThroughput|TestAccVPCAccessConnector_vpcAccessConnectorThroughput_maxThroughput

Get to know how VCR tests work

modular-magician avatar Jun 10 '24 15:06 modular-magician

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

modular-magician avatar Jun 10 '24 15:06 modular-magician

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.

google provider: Diff ( 8 files changed, 176 insertions(+), 6 deletions(-)) google-beta provider: Diff ( 8 files changed, 176 insertions(+), 6 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 2 insertions(+)) Open in Cloud Shell: Diff ( 2 files changed, 4 insertions(+))

Breaking Change(s) Detected

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

  • Field max_throughput default value changed from 300 to on google_vpc_access_connector - reference
  • Field min_throughput default value changed from 200 to on google_vpc_access_connector - 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.

modular-magician avatar Jun 10 '24 16:06 modular-magician

Tests analytics

Total tests: 37 Passed tests: 31 Skipped tests: 1 Affected tests: 5

Click here to see the affected service packages
  • appengine
  • cloudfunctions
  • vpcaccess

Action taken

Found 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccVPCAccessConnector_vpcAccessConnectorExample|TestAccVPCAccessConnector_vpcAccessConnectorSharedVpcExample|TestAccVPCAccessConnector_vpcAccessConnectorThroughput|TestAccVPCAccessConnector_vpcAccessConnectorThroughput_combiningThroughputAndInstancesFields_permadiff|TestAccVPCAccessConnector_vpcAccessConnectorThroughput_usingThroughputOrInstancesLimits

Get to know how VCR tests work

modular-magician avatar Jun 10 '24 16:06 modular-magician

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$ TestAccVPCAccessConnector_vpcAccessConnectorSharedVpcExample[Debug log] TestAccVPCAccessConnector_vpcAccessConnectorThroughput[Debug log] TestAccVPCAccessConnector_vpcAccessConnectorThroughput_combiningThroughputAndInstancesFields_permadiff[Debug log]

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


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

modular-magician avatar Jun 10 '24 17:06 modular-magician

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.

google provider: Diff ( 8 files changed, 176 insertions(+), 6 deletions(-)) google-beta provider: Diff ( 8 files changed, 176 insertions(+), 6 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 2 insertions(+)) Open in Cloud Shell: Diff ( 2 files changed, 4 insertions(+))

Breaking Change(s) Detected

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

  • Field max_throughput default value changed from 300 to on google_vpc_access_connector - reference
  • Field min_throughput default value changed from 200 to on google_vpc_access_connector - 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.

modular-magician avatar Jun 10 '24 17:06 modular-magician

Tests analytics

Total tests: 37 Passed tests: 34 Skipped tests: 1 Affected tests: 2

Click here to see the affected service packages
  • appengine
  • cloudfunctions
  • vpcaccess

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
TestAccVPCAccessConnector_vpcAccessConnectorExample|TestAccVPCAccessConnector_vpcAccessConnectorThroughput_usingThroughputOrInstancesLimits

Get to know how VCR tests work

modular-magician avatar Jun 10 '24 18:06 modular-magician

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

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


$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$ TestAccVPCAccessConnector_vpcAccessConnectorExample[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

modular-magician avatar Jun 10 '24 18:06 modular-magician

That last test is failing due to a networking reason:

    vcr_utils.go:152: Step 1/2 error: Error running apply: exit status 1
        
        Error: Error waiting to create Connector: Error waiting for Creating Connector: Error code 3, message: Operation failed: Invalid IP CIDR range was provided. It conflicts with an existing subnetwork. Please delete the connector manually.
        
          with google_vpc_access_connector.connector,
          on terraform_plugin_test.tf line 2, in resource "google_vpc_access_connector" "connector":
           2: resource "google_vpc_access_connector" "connector" {

SarahFrench avatar Jun 10 '24 18:06 SarahFrench

As a recap of this PR's stages:

  1. Originally contained only an acceptance test demonstrating the permadiff in the linked issue f7bce51 - test failure due to permadiff here

  2. Updated the provider (@ 26ca216) to:

  • remove default values on min_ and max_throughput, then needed to make those fields default from the API because all 4 fields are always present
  • update existing tests to no longer rely on the default values for min_ and max_throughput
  • update the tests (in https://github.com/GoogleCloudPlatform/magic-modules/pull/10313/commits/fe60725f36152a6e2c469bbc64c6286be0abddee )related to the these fields' weird behaviour/the permadiff to show that setting all 4 fields results in a permadiff and that when 2 fields are set the other 2 fields are present.

SarahFrench avatar Jun 10 '24 18:06 SarahFrench

In the commits above I stop users from being able to include both min_throughput and min_instance (same for max_) by making fields conflict. Previously users would experience a permadiff, but this change means that users will fix their config to avoid the permadiff.

SarahFrench avatar Jun 10 '24 19:06 SarahFrench

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.

google provider: Diff ( 8 files changed, 182 insertions(+), 6 deletions(-)) google-beta provider: Diff ( 8 files changed, 182 insertions(+), 6 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 2 insertions(+)) Open in Cloud Shell: Diff ( 2 files changed, 4 insertions(+))

Breaking Change(s) Detected

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

  • Field max_throughput default value changed from 300 to on google_vpc_access_connector - reference
  • Field min_throughput default value changed from 200 to on google_vpc_access_connector - 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.

modular-magician avatar Jun 10 '24 19:06 modular-magician

Tests analytics

Total tests: 37 Passed tests: 34 Skipped tests: 1 Affected tests: 2

Click here to see the affected service packages
  • appengine
  • cloudfunctions
  • vpcaccess

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
TestAccVPCAccessConnector_vpcAccessConnectorExample|TestAccVPCAccessConnector_vpcAccessConnectorThroughput_combiningThroughputAndInstancesFields_conflict

Get to know how VCR tests work

modular-magician avatar Jun 10 '24 19:06 modular-magician

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

$\textcolor{red}{\textsf{Tests failed when rerunning REPLAYING mode:}}$ TestAccVPCAccessConnector_vpcAccessConnectorThroughput_combiningThroughputAndInstancesFields_conflict[Error message] [Debug log]

Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made.

Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer.


$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$ TestAccVPCAccessConnector_vpcAccessConnectorExample[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

modular-magician avatar Jun 10 '24 19:06 modular-magician

Ah, we cannot have acc tests that don't reach out to an API run in VCR:

    test_utils.go:108: no cassette found on disk for TestAccVPCAccessConnector_vpcAccessConnectorThroughput_combiningThroughputAndInstancesFields_conflict, please replay this testcase in recording mode - open /workspace/.ci/magician/cassettes/beta/TestAccVPCAccessConnector_vpcAccessConnectorThroughput_combiningThroughputAndInstancesFields_conflict.seed: no such file or directory

SarahFrench avatar Jun 11 '24 11:06 SarahFrench

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.

google provider: Diff ( 8 files changed, 185 insertions(+), 6 deletions(-)) google-beta provider: Diff ( 8 files changed, 185 insertions(+), 6 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 2 insertions(+)) Open in Cloud Shell: Diff ( 2 files changed, 4 insertions(+))

Breaking Change(s) Detected

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

  • Field max_throughput default value changed from 300 to on google_vpc_access_connector - reference
  • Field min_throughput default value changed from 200 to on google_vpc_access_connector - 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.

modular-magician avatar Jun 11 '24 16:06 modular-magician

Tests analytics

Total tests: 37 Passed tests: 34 Skipped tests: 2 Affected tests: 1

Click here to see the affected service packages
  • appengine
  • cloudfunctions
  • vpcaccess

Non-exercised tests

Tests were added that are skipped in VCR:

  • TestAccVPCAccessConnector_vpcAccessConnectorThroughput_combiningThroughputAndInstancesFields_conflict

Action taken

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

Get to know how VCR tests work

modular-magician avatar Jun 11 '24 16:06 modular-magician

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$ TestAccVPCAccessConnector_vpcAccessConnectorExample[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

modular-magician avatar Jun 11 '24 16:06 modular-magician