-
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
Allow updating of resource policies in compute_disk #2228
Allow updating of resource policies in compute_disk #2228
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Signed-off-by: Chris Sng <[email protected]>
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs. Thanks for your contribution! A human will be with you soon. @rileykarson, please review this PR or find an appropriate assignee. |
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs. Thanks for your contribution! A human will be with you soon. @SirGitsalot, please review this PR or find an appropriate assignee. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in terraform-google-conversion. New Pull RequestsI built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
@rileykarson Thank you for the suggestions, I will work on them. I realise that the update is not fully addressed. According to the Beta API, there is also the removeResourcePolicies and not just the addResourcePolicies API. This would suggest that custom update code has to be done on resource_policies property updates that calls both the APIs. It has to handle cases:
In case (1), no need to call removeResourcePolicies. In case (2), need to call removeResourcePolicies only. In case (3), need to call removeResourcePolicies first before calling addResourcePolicies. Looking at the magic-modules documentation and codelab, I have not been successful incorporating these behavior using |
… instead - custom pre_delete code to include resourcePolicies in POST body
Right, sorry about that. I managed to miss that it was an https://github.com/GoogleCloudPlatform/magic-modules/pull/1846/files is a good sample of what adding one looks like. https://github.com/GoogleCloudPlatform/magic-modules/pull/1504/files#diff-fb4f76e7d870258668a3beac48bf164cR276 is also useful as a reference, but it contains some unrelated changes. Edit: Ah, but resource policies are primitives and not nested objects. This is case we haven't handled previously. I'll dig a bit more. |
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 looked into the API more closely. The addX
/removeX
endpoints expecting a list instead of a single element is unusual, but we should be able to express everything cleanly other than that, and use custom code (an encoder) to pretend the API looks how we'd expect.
I haven't personally written a fine-grained resource, so it's very possible that I've missed something. If you encounter any issues or need any clarifications (or otherwise get stuck), please comment and I can provide feedback!
Co-Authored-By: Riley Karson <[email protected]>
…rissng/magic-modules into compute-disk-update-resource-policies
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.
It looks like we're just failing to import the resource. I think we need to massage the import id slightly by changing the base_url
value. MM constructs the id based on pre-existing data when it can: https://github.com/GoogleCloudPlatform/magic-modules/blob/master/provider/terraform/import.rb#L36
templates/terraform/encoders/compute_disk_resource_policies_attachment.go.erb
Outdated
Show resolved
Hide resolved
templates/terraform/decoders/compute_disk_resource_policies_attachment.go.erb
Outdated
Show resolved
Hide resolved
We're good; the tests passed! 🙏 |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
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.
Just a couple small nitpicks left, overall LGTM.
Can you remove google-beta
from https://github.com/GoogleCloudPlatform/magic-modules/blob/master/templates/terraform/examples/resource_policy_basic.tf.erb and https://github.com/GoogleCloudPlatform/magic-modules/blob/master/templates/terraform/examples/resource_policy_full.tf.erb?
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of 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.
LGTM! Thanks for your patience in working through adding a fine-grained resource like this @chrissng, we haven't added too many so the patterns aren't very well-defined yet.
Do you mind giving explicit consent to the CLA under GoogleCloudPlatform/terraform-google-conversion#192? While the PR is authored by the our CI robot user, we attribute the PR to the upstream author and our CLA requires explicit consent from committers.
@rileykarson I would say this would not have been possible without your help. Thank you for your patience and guidance throughout this PR! |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
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.
Sorry! One small thing popped up after your latest merge.
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
This should be merged soon, our generator attached the wrong downstream changes to this PR yesterday. This next run (should) fix it, and it it doesn't I'll reconcile manually. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
Edited above to point to the original conversion PR. |
Allow updating of resource policies in compute_disk by using an attachment resource.
TODOs:
Release Note for Downstream PRs (will be copied)