-
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
Finish converting ACM resources to use policy mutex lock #12735
base: main
Are you sure you want to change the base?
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @hao-nan-li, 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. |
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.
|
1 similar comment
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: 3 Click here to see the affected service packages
🟢 All tests passed! View the build log |
Tests analyticsTotal tests: 3 Click here to see the affected service packages
🟢 All tests passed! View the build log |
type: String | ||
description: | | ||
The name of the Access Policy this resource belongs to. | ||
ignore_read: true |
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.
Why is this field immutable
and output true
at the same time?
Can the user set the policyId from their TF config?
This comment also applies for all the cases below.
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.
Output
because the field should never be set by the user. It's a substring automatically parsed from the perimeter
parameter they set (varies slightly across different resources).
Ie users are setting a perimeter
parameter to accessPolicies/123/servicePerimeters/abc
and we now want the mutex to be the substring accessPolicies/123
. I want this to be an invisible change for users, so instead of having them input another param we can just parse it from that.
Immutable
since the accessPolicy
can never change once it's set.
If there's an easier way to have a mutex lock on a substring of a parameter, please let me know. I was kind of hoping we'd establish the pattern in #12725 and then this PR was just copying that to all other resources, but if we need changes that's fine.
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 the explanation.
If immutable
is set to true, if there's any modification to this field, another new resource will be recreated. Is that something you'd expect?
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.
Ah I think I see what you're saying. I wouldn't want that to trigger a recreate. Really I want the field to be ignored except where I'm reading it in the pre_create
. I've removed the immutable
property from all of them but kept output
since users should not input data themselves into the property. Thanks for the help!
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: 3 Click here to see the affected service packages
🟢 All tests passed! View the build log |
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: 3 Click here to see the affected service packages
🟢 All tests passed! View the build log |
This is a continuation of #12725 to migrate all ACM resources to use a policy level mutex lock. Some resources already have a policy field, so I just used that. Others don't have one so I created a new access_policy_id property and populate it in the encoder. Note, I've signed up for Tuesday office hours to discuss ideas on simplifying this, but I'd prefer to not block this PR on that. Also, I think it's important that both #12725 and this PR go in the same release since migrating some resources to a different lock and not others might cause issues. Thanks!
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.