-
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
Add resource manager tags to GKE autopilot #10123
Add resource manager tags to GKE autopilot #10123
Conversation
…r-to-cluster sync merge from upstream/main
…r-to-cluster sync merge from upstream/main
…r-to-cluster sync merge from upstream/main
…r-to-cluster sync merge from upstream/main
…r-to-cluster syn merge from upstream/main
…r-to-cluster sync merge from upstream/main
sync merge from main
…r-to-cluster sync merge from upstream/main
…r-to-cluster sync merge from upstream/main
…r-to-cluster sync merge from upstream/main
…r-to-cluster sync merge from upstream/main
…r-to-cluster sync merge from upstream/main
…r-to-cluster sync merge from upstream/main
|
Hello @SarahFrench could u tell me if there is something missing to merge this PR? |
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.
This PR looks good given the tests for the new field. There's a small refactor that we'd need before merging, to make sure the new test doesn't impact other acceptance tests.
mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.erb
Show resolved
Hide resolved
…r-tags-to-autopilot sync merge from upstream/main
Hello @SarahFrench, I updated the PR considering your suggestions, could you run the tests again please? |
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.
Thanks for doing the refactoring I suggested- I noticed some problems with use of the new _iam_membership resources and I left some comments. Those comments may need to be applied in other places where those resources are used.
members = [ | ||
"serviceAccount:service-${data.google_project.project.number}@container-engine-robot.iam.gserviceaccount.com", | ||
] |
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.
members = [ | |
"serviceAccount:service-${data.google_project.project.number}@container-engine-robot.iam.gserviceaccount.com", | |
] | |
member = "serviceAccount:service-${data.google_project.project.number}@container-engine-robot.iam.gserviceaccount.com" |
IAM membership resources manage only one membership in a binding, so cannot take a list as an argument here.
] | ||
} | ||
|
||
resource "google_project_iam_member" "tagUser" { |
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.
I'm afraid there'll need to be two membership resources used here to replace the original binding resource; _iam_membership resources only take a singular member argument
Hello @SarahFrench can I just roll back to google_project_iam_binding? I ve got the strong feeling I will mess up with the replaying mode. |
Ok, roll back and I can update your PR to use _iam_member resources after it's merged. Even though you got a previous PR accepted while using _iam_binding resources that doesn't mean that it's ideal. Once your new test joins our full test suite it will have the potential to conflict with other tests that also try to create bindings for that role at the project level. This type of conflict is what the _iam_member resources are designed to avoid. |
This reverts commit 55ebc03. undo non-authoritative refactor
Hello @SarahFrench, thanks for the feedback. I reverted the changes to the commit where tests were passing. |
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
|
Adds support for Resource Manager Tags for Autopilot GKE cluster resource
Part of: hashicorp/terraform-provider-google#16614
Release Note Template for Downstream PRs (will be copied)