fixed artifact_registry_repository permadiff in unordered array #9376

Subserial · 2023-10-27T19:06:06Z

Suppress diff of artifact_registry_repository.virtual_repository_config.upstream_policies order using set semantics.
Fixes hashicorp/terraform-provider-google#16381

Release Note Template for Downstream PRs (will be copied)

artifactregistry: fixed permadiff due to unsorted `virtual_repository_config` array in `google_artifact_registry_repository`

modular-magician · 2023-10-27T19:07:39Z

Hello! I am a robot. It looks like you are a: Community Contributor ~~Googler~~ ~~Core Contributor~~. Tests will require approval to run.

@trodge, 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.

Subserial · 2023-10-27T19:12:16Z

How do I get magician to read my account as a Googler?

trodge · 2023-10-30T20:11:39Z

How do I get magician to read my account as a Googler?

The magician would have checked your membership using the GitHub api for the googlers and GoogleCloudPlatform orgs. If you recently made your membership public perhaps it had not yet reached the API when the magician made the request.

Sorry for the delay.

modular-magician · 2023-10-30T20:22:01Z

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 virtual_repository_config.upstream_policies changed from TypeList to TypeSet on google_artifact_registry_repository - 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 ( 1 file changed, 29 insertions(+), 23 deletions(-))
Terraform Beta: Diff ( 1 file changed, 29 insertions(+), 23 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2023-10-30T21:34:01Z

Tests analytics

Total tests: 3184
Passed tests 2859
Skipped tests: 325
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

melinath · 2023-10-31T17:05:53Z

@Subserial drive-by comment: see go/terraform-contribution-guide#before-you-begin for more details

trodge

Unfortunately, you will need to find a different way to fix this bug, since making a list into a set would be a breaking change for existing users. See this comment for details.

When such a way is found, please re-request review on this PR.

modular-magician · 2023-11-06T23:09:09Z

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 ( 1 file changed, 44 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 1 file changed, 44 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 42 insertions(+))

modular-magician · 2023-11-06T23:33:45Z

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 ( 1 file changed, 44 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 1 file changed, 44 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 42 insertions(+))

modular-magician · 2023-11-07T00:46:28Z

Tests analytics

Total tests: 3206
Passed tests 2880
Skipped tests: 325
Affected tests: 1

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

TestAccLoggingProjectSink_updatePreservesCustomWriter

Get to know how VCR tests work

modular-magician · 2023-11-07T00:51:40Z

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

Rerun these tests in REPLAYING mode to catch issues

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

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

Subserial · 2023-11-08T05:00:34Z

I'm not sure what the requested change feature in Github is, but otherwise I've updated the strategy for addressing the permadiff.

trodge

"Request changes" just means make changes to your PR and then re-request review when you want your reviewer to take another look.

Edit- also take a look at my third comment before addressing the other two, they didn't come out in the order I expected.

trodge · 2023-11-08T23:38:06Z

mmv1/templates/terraform/constants/artifact_registry_repository.go.erb

+  }
+  hashFunc := func(val any) string {
+    x := val.(map[string]any)
+    return fmt.Sprintf("[id:%v priority:%v repository:%v]", x["id"], x["priority"], x["repository"])


I believe this is equivalent to

return fmt.Sprintf("%v", x)

I'm not sure if golang is consistent with map key order within the same process. This was to prevent two maps from outputting different strings with the same information.

trodge · 2023-11-08T23:39:32Z

mmv1/templates/terraform/constants/artifact_registry_repository.go.erb

+}
+
+func arrayDiffSuppress(old, new []any, hashFunc func(x any) string) bool {
+  oldMap := map[string]bool{}


If we're just making a set here we can use map[string]struct{}, if we need a boolean value that's distinct from omitting the key, use map[string]bool.

Replaced with schema.Set.

trodge · 2023-11-08T23:53:58Z

mmv1/templates/terraform/constants/artifact_registry_repository.go.erb

+  # limitations under the License.
+-%>
+func upstreamPoliciesDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
+  log.Printf("%v, %v, %v\n", k, old, new)


Unless we have a reason to do this by hand, it would probably be better to use the sdk provided set methods such as schema.NewSet and Set.Equal to ensure we don't miss anything with casting the types around.

Agreed! Switched over to using schema.Set.

modular-magician · 2023-11-14T21:53:34Z

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 ( 1 file changed, 24 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 1 file changed, 24 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 21 insertions(+))

modular-magician · 2023-11-14T23:13:20Z

Tests analytics

Total tests: 3226
Passed tests 2893
Skipped tests: 329
Affected tests: 4

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

TestAccDataprocClusterIamPolicy|TestAccDataprocJobIamPolicy|TestAccLoggingProjectSink_updatePreservesCustomWriter|TestAccDataSourceGoogleServiceAccountJwt

Get to know how VCR tests work

modular-magician · 2023-11-14T23:24:33Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[Debug log]
TestAccDataprocJobIamPolicy[Debug log]
TestAccDataSourceGoogleServiceAccountJwt[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:}}$
TestAccLoggingProjectSink_updatePreservesCustomWriter[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

trodge

LGTM, but it looks like the test isn't running in VCR. Would it be possible to modify an existing test so that it will be re-recorded? Ideally we would just add another element to upstream_policies to make sure the diff suppress works as expected.

trodge · 2023-11-15T16:02:49Z

mmv1/templates/terraform/constants/artifact_registry_repository.go.erb

+  }
+  hashFunc := func(val any) int {
+    x := val.(map[string]any)
+    hashStr := fmt.Sprintf("[id:%v priority:%v repository:%v]", x["id"], x["priority"], x["repository"])


Is there any chance we could see a panic if val isn't a map?

It is possible if the returned field doesn't fit the expected schema, somehow. I updated it to avoid panicking on a bad cast.

modular-magician · 2023-11-18T01:42:04Z

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, 60 insertions(+), 14 deletions(-))
Terraform Beta: Diff ( 3 files changed, 60 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 21 insertions(+))
TF OiCS: Diff ( 1 file changed, 18 insertions(+), 6 deletions(-))

modular-magician · 2023-11-18T01:51:06Z

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, 73 insertions(+), 14 deletions(-))
Terraform Beta: Diff ( 3 files changed, 73 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 34 insertions(+))
TF OiCS: Diff ( 1 file changed, 18 insertions(+), 6 deletions(-))

modular-magician · 2023-11-18T03:02:31Z

Tests analytics

Total tests: 3236
Passed tests 2902
Skipped tests: 330
Affected tests: 4

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

TestAccArtifactRegistryRepository_artifactRegistryRepositoryVirtualExample|TestAccLoggingProjectSink_updatePreservesCustomWriter|TestAccDataSourceGoogleServiceAccountJwt|TestAccDataSourceGoogleServiceAccountIdToken_impersonation

Get to know how VCR tests work

modular-magician · 2023-11-18T03:09:31Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccArtifactRegistryRepository_artifactRegistryRepositoryVirtualExample[Debug log]
TestAccDataSourceGoogleServiceAccountJwt[Debug log]
TestAccDataSourceGoogleServiceAccountIdToken_impersonation[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:}}$
TestAccLoggingProjectSink_updatePreservesCustomWriter[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

Subserial · 2023-11-27T19:10:44Z

The failed test is not caused by this pull request.

…leCloudPlatform#9376) * fixed permadiff in unordered array * changed permadiff fix strategy for upstream policies * removed no-op field * change diff semantics to use schema.Set * updated virtual example with intentional out-of-order priorities * update virtual policy comparison to not panic on bad cast (if possible)

fixed permadiff in unordered array

5d2ec71

modular-magician requested a review from trodge October 27, 2023 19:07

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Oct 27, 2023

modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Oct 30, 2023

trodge requested changes Oct 31, 2023

View reviewed changes

changed permadiff fix strategy for upstream policies

27fe321

modular-magician requested a review from trodge November 6, 2023 22:59

modular-magician added the service/artifactregistry label Nov 6, 2023

Subserial added 2 commits November 6, 2023 15:19

Merge branch 'main' into ar-virtual-permadiff

f9607ee

removed no-op field

8a2d5cc

trodge requested changes Nov 8, 2023

View reviewed changes

change diff semantics to use schema.Set

5d80838

modular-magician requested a review from trodge November 14, 2023 21:39

Merge branch 'main' into ar-virtual-permadiff

3bc1882

trodge reviewed Nov 15, 2023

View reviewed changes

Merge branch 'main' into ar-virtual-permadiff

c03db5f

updated virtual example with intentional out-of-order priorities

f3c2461

modular-magician requested a review from trodge November 18, 2023 01:32

update virtual policy comparison to not panic on bad cast (if possible)

f66a055

trodge approved these changes Dec 1, 2023

View reviewed changes

trodge merged commit 9988136 into GoogleCloudPlatform:main Dec 1, 2023
14 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed artifact_registry_repository permadiff in unordered array #9376

fixed artifact_registry_repository permadiff in unordered array #9376

Subserial commented Oct 27, 2023 •

edited by trodge

Loading

modular-magician commented Oct 27, 2023

Subserial commented Oct 27, 2023

trodge commented Oct 30, 2023

modular-magician commented Oct 30, 2023

modular-magician commented Oct 30, 2023

melinath commented Oct 31, 2023

trodge left a comment

modular-magician commented Nov 6, 2023

modular-magician commented Nov 6, 2023

modular-magician commented Nov 7, 2023

modular-magician commented Nov 7, 2023

Subserial commented Nov 8, 2023

trodge left a comment •

edited

Loading

trodge Nov 8, 2023

Subserial Nov 15, 2023

trodge Nov 8, 2023

Subserial Nov 15, 2023

trodge Nov 8, 2023

Subserial Nov 15, 2023

modular-magician commented Nov 14, 2023

modular-magician commented Nov 14, 2023

modular-magician commented Nov 14, 2023

trodge left a comment

trodge Nov 15, 2023

Subserial Nov 18, 2023

modular-magician commented Nov 18, 2023

modular-magician commented Nov 18, 2023

modular-magician commented Nov 18, 2023

modular-magician commented Nov 18, 2023

Subserial commented Nov 27, 2023

fixed artifact_registry_repository permadiff in unordered array #9376

fixed artifact_registry_repository permadiff in unordered array #9376

Conversation

Subserial commented Oct 27, 2023 • edited by trodge Loading

modular-magician commented Oct 27, 2023

Subserial commented Oct 27, 2023

trodge commented Oct 30, 2023

modular-magician commented Oct 30, 2023

Breaking Change(s) Detected

Diff report

modular-magician commented Oct 30, 2023

Tests analytics

melinath commented Oct 31, 2023

trodge left a comment

Choose a reason for hiding this comment

modular-magician commented Nov 6, 2023

Diff report

modular-magician commented Nov 6, 2023

Diff report

modular-magician commented Nov 7, 2023

Tests analytics

Action taken

modular-magician commented Nov 7, 2023

Rerun these tests in REPLAYING mode to catch issues

Subserial commented Nov 8, 2023

trodge left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Nov 14, 2023

Diff report

modular-magician commented Nov 14, 2023

Tests analytics

Action taken

modular-magician commented Nov 14, 2023

Rerun these tests in REPLAYING mode to catch issues

trodge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Nov 18, 2023

Diff report

modular-magician commented Nov 18, 2023

Diff report

modular-magician commented Nov 18, 2023

Tests analytics

Action taken

modular-magician commented Nov 18, 2023

Rerun these tests in REPLAYING mode to catch issues

Subserial commented Nov 27, 2023

Subserial commented Oct 27, 2023 •

edited by trodge

Loading

trodge left a comment •

edited

Loading