The version field should be under configmanagement instead of under oci #9587

sahsagar-google · 2023-12-05T22:53:50Z

Fixes hashicorp/terraform-provider-google#16684

Release Note Template for Downstream PRs (will be copied)

gkehub2: added field `version` under `configmanagement` instead of a child field `oci` in `google_gke_hub_feature` resource

…hings simpler in the review

…h the one from test env

…rted

sahsagar-google · 2023-12-07T21:55:06Z

What happens when users put the version inside oci in the configuration? Does API return an error? @sahsagar-google

Yes, an error is returned. Something like this

Error: Error updating Feature "projects/sagarsahai-gke-dev/locations/global/features/configmanagement": googleapi: Error 400: Invalid JSON payload received. Unknown name "version" at 'resource.fleet_default_member_config.configmanagement.config_sync.oci': Cannot find field.
│ Details:
│ [
│ {
│ "@type": "type.googleapis.com/google.rpc.BadRequest",
│ "fieldViolations": [
│ {
│ "description": "Invalid JSON payload received. Unknown name "version" at 'resource.fleet_default_member_config.configmanagement.config_sync.oci': Cannot find field.",
│ "field": "resource.fleet_default_member_config.configmanagement.config_sync.oci"
│ }
│ ]
│ }
│ ]

zli82016 · 2023-12-07T22:02:56Z

What happens when users put the version inside oci in the configuration? Does API return an error? @sahsagar-google

Yes, an error is returned. Something like this

Error: Error updating Feature "projects/sagarsahai-gke-dev/locations/global/features/configmanagement": googleapi: Error 400: Invalid JSON payload received. Unknown name "version" at 'resource.fleet_default_member_config.configmanagement.config_sync.oci': Cannot find field.
│ Details:
│ [
│ {
│ "@type": "type.googleapis.com/google.rpc.BadRequest",
│ "fieldViolations": [
│ {
│ "description": "Invalid JSON payload received. Unknown name "version" at 'resource.fleet_default_member_config.configmanagement.config_sync.oci': Cannot find field.",
│ "field": "resource.fleet_default_member_config.configmanagement.config_sync.oci"
│ }
│ ]
│ }
│ ]

Thanks for the clarification. Can you please add the test for the version field in the file https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/services/gkehub2/resource_gke_hub_feature_test.go.erb#L456?

zli82016 · 2023-12-07T22:05:41Z

It is not a breaking change as it returns error when version field is in the original place.

mmv1/third_party/terraform/services/gkehub2/resource_gke_hub_feature_test.go.erb

modular-magician · 2023-12-12T22:00:47Z

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 fleet_default_member_config.configmanagement.config_sync.oci.version within resource google_gke_hub_feature was either removed or renamed - 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 ( 3 files changed, 28 insertions(+), 26 deletions(-))
Terraform Beta: Diff ( 3 files changed, 28 insertions(+), 26 deletions(-))
TF Conversion: Diff ( 1 file changed, 11 insertions(+), 11 deletions(-))

modular-magician · 2023-12-12T22:28:55Z

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 fleet_default_member_config.configmanagement.config_sync.oci.version within resource google_gke_hub_feature was either removed or renamed - 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 ( 3 files changed, 28 insertions(+), 26 deletions(-))
Terraform Beta: Diff ( 3 files changed, 28 insertions(+), 26 deletions(-))
TF Conversion: Diff ( 1 file changed, 11 insertions(+), 11 deletions(-))

modular-magician · 2023-12-12T23:27:58Z

Tests analytics

Total tests: 3292
Passed tests 2953
Skipped tests: 338
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

TestAccSpannerDatabaseIamPolicy

Get to know how VCR tests work

modular-magician · 2023-12-12T23:31:17Z

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

zli82016 · 2023-12-12T23:40:06Z

Can you please run the test TestAccGKEHubFeature_FleetDefaultMemberConfigConfigManagement locally to see if it will pass or not? It is skipped in VCR tests.

zli82016

LGTM. The contributor ran the test locally and it passed.

… under oci (#9587)" This reverts commit de7d39b.

shuyama1 · 2023-12-14T18:52:05Z

@sahsagar-google @zli82016 This is still a breaking that we should avoid. We may consider - deprecating configmanagement.config_sync.oci.version and adding configmanagement.version for now and wait until next major release to remove configmanagement.config_sync.oci.version. Do we see any issues deprecating the incorrect field for now and removing it in the next major?

zli82016 · 2023-12-14T18:57:53Z

@sahsagar-google @zli82016 This is still a breaking that we should avoid. We may consider - deprecating configmanagement.config_sync.oci.version and adding configmanagement.version for now and wait until next major release to remove configmanagement.config_sync.oci.version. Do we see any issues deprecating the incorrect field for now and removing it in the next major?

Thanks for reverting the PR, @shuyama1. It makes sense to me. Previously I thought it is OK to remove configmanagement.config_sync.oci.version as it always returns an error.

zli82016 · 2023-12-14T19:08:06Z

Sorry, @sahsagar-google , can you please open a new PR to deprecate configmanagement.config_sync.oci.version and add configmanagement.version? Thanks.

…ci (GoogleCloudPlatform#9587) * Adding Terraform resources for Tenancy APIs in GKEHub * Segregating MembershipBinding and MembershipRBACRoleBinding to keep things simpler in the review * Fixing the docu URIs * Adding TF support for Tenancy API for Membership Binding * Adding dependent membership binding to the same commit chain * Making Scope un-updatable and replacing hard coded project number with the one from test env * Making Scope RRBAC updatable * Making Namespace immutable * Adding update test cases * Removing all memberships field from Scope since it is no longer supported * Removing all_memberships field for Scope from all test cases * Making naming in examples consistent across Tenancy APIs documentation * Update mmv1/templates/terraform/examples/gkehub_membership_binding_basic.tf.erb Co-authored-by: Shuya Ma <[email protected]> * Update mmv1/templates/terraform/examples/gkehub_membership_binding_basic.tf.erb Co-authored-by: Shuya Ma <[email protected]> * Update mmv1/templates/terraform/examples/gkehub_membership_rbac_role_binding_basic.tf.erb Co-authored-by: Shuya Ma <[email protected]> * Fixing typo in the resource name * Adding state migrations for Membership * Updating the feature_membership documentation with the current resource state. Adding membership_location field to the doc * Fixing the version field to be directly under configmanagement instead of under the oci field * Adding tests for the field version * Changing version fields value to test upgradation --------- Co-authored-by: Shuya Ma <[email protected]>

sahsagar-google and others added 30 commits July 19, 2023 19:04

Adding Terraform resources for Tenancy APIs in GKEHub

85298f1

Merge branch 'GoogleCloudPlatform:main' into main

05f9e1d

Merge branch 'GoogleCloudPlatform:main' into main

d056843

Segregating MembershipBinding and MembershipRBACRoleBinding to keep t…

20fea52

…hings simpler in the review

Merge branch 'GoogleCloudPlatform:main' into main

672494c

Merge branch 'GoogleCloudPlatform:main' into main

3a4cefa

Fixing the docu URIs

693aaff

Merge branch 'GoogleCloudPlatform:main' into main

1d65988

Merge branch 'GoogleCloudPlatform:main' into main

d783ddd

Adding TF support for Tenancy API for Membership Binding

bc96cbb

Adding dependent membership binding to the same commit chain

f74ce0f

Making Scope un-updatable and replacing hard coded project number wit…

e0c2212

…h the one from test env

Merge branch 'GoogleCloudPlatform:main' into main

c77f8b2

Making Scope RRBAC updatable

71c92e6

Merge branch 'GoogleCloudPlatform:main' into main

2403caa

Merge branch 'GoogleCloudPlatform:main' into main

308091c

Merge branch 'GoogleCloudPlatform:main' into main

a0f99d5

Merge branch 'GoogleCloudPlatform:main' into main

78c9611

Making Namespace immutable

45ee3fc

Merge branch 'GoogleCloudPlatform:main' into main

4852207

Merge branch 'GoogleCloudPlatform:main' into main

79a8e93

Adding update test cases

db46dd0

Merge branch 'GoogleCloudPlatform:main' into main

1cb54db

Removing all memberships field from Scope since it is no longer suppo…

f574890

…rted

Merge branch 'GoogleCloudPlatform:main' into main

35ea32e

Removing all_memberships field for Scope from all test cases

dd80ce3

Merge branch 'GoogleCloudPlatform:main' into main

371905f

Merge branch 'GoogleCloudPlatform:main' into main

ad76f8b

Merge branch 'GoogleCloudPlatform:main' into main

3d51fc1

Merge branch 'GoogleCloudPlatform:main' into main

61ed74c

zli82016 added the override-breaking-change Allows a potential breaking change to be merged label Dec 7, 2023

sahsagar-google and others added 2 commits December 12, 2023 21:48

Adding tests for the field version

6cd2070

Merge branch 'GoogleCloudPlatform:main' into main

7dc3a1f

zli82016 reviewed Dec 12, 2023

View reviewed changes

mmv1/third_party/terraform/services/gkehub2/resource_gke_hub_feature_test.go.erb Outdated Show resolved Hide resolved

Changing version fields value to test upgradation

14b6fc2

modular-magician requested a review from zli82016 December 12, 2023 22:19

zli82016 approved these changes Dec 12, 2023

View reviewed changes

zli82016 merged commit de7d39b into GoogleCloudPlatform:main Dec 12, 2023
15 checks passed

shuyama1 added a commit that referenced this pull request Dec 14, 2023

Revert "The version field should be under configmanagement instead of…

e98da3a

… under oci (#9587)" This reverts commit de7d39b.

shuyama1 mentioned this pull request Dec 14, 2023

Revert "The version field should be under configmanagement instead of under oci" #9645

Merged

sahsagar-google mentioned this pull request Dec 14, 2023

The version field should be under configmanagement instead of under oci, deprecating the older field #9648

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The version field should be under configmanagement instead of under oci #9587

The version field should be under configmanagement instead of under oci #9587

sahsagar-google commented Dec 5, 2023 •

edited by zli82016

Loading

sahsagar-google commented Dec 7, 2023

zli82016 commented Dec 7, 2023 •

edited

Loading

zli82016 commented Dec 7, 2023

modular-magician commented Dec 12, 2023

modular-magician commented Dec 12, 2023

modular-magician commented Dec 12, 2023

modular-magician commented Dec 12, 2023

zli82016 commented Dec 12, 2023

zli82016 left a comment •

edited

Loading

shuyama1 commented Dec 14, 2023

zli82016 commented Dec 14, 2023

zli82016 commented Dec 14, 2023 •

edited

Loading

The version field should be under configmanagement instead of under oci #9587

The version field should be under configmanagement instead of under oci #9587

Conversation

sahsagar-google commented Dec 5, 2023 • edited by zli82016 Loading

sahsagar-google commented Dec 7, 2023

zli82016 commented Dec 7, 2023 • edited Loading

zli82016 commented Dec 7, 2023

modular-magician commented Dec 12, 2023

Breaking Change(s) Detected

Diff report

modular-magician commented Dec 12, 2023

Breaking Change(s) Detected

Diff report

modular-magician commented Dec 12, 2023

Tests analytics

Action taken

modular-magician commented Dec 12, 2023

Rerun these tests in REPLAYING mode to catch issues

zli82016 commented Dec 12, 2023

zli82016 left a comment • edited Loading

Choose a reason for hiding this comment

shuyama1 commented Dec 14, 2023

zli82016 commented Dec 14, 2023

zli82016 commented Dec 14, 2023 • edited Loading

sahsagar-google commented Dec 5, 2023 •

edited by zli82016

Loading

zli82016 commented Dec 7, 2023 •

edited

Loading

zli82016 left a comment •

edited

Loading

zli82016 commented Dec 14, 2023 •

edited

Loading