-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Users API changes #8920
Users API changes #8920
Conversation
Hello! I am a robot. It looks like you are a: @shuyama1, 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. Terraform GA: Diff ( 5 files changed, 1063 insertions(+), 2 deletions(-)) |
Remove the trailing empty line
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. Terraform GA: Diff ( 5 files changed, 1063 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbUser_updateRoles_IAM|TestAccAlloydbUser_updatePassword_BuiltIn|TestAccAlloydbUser_updateRoles_BuiltIn|TestAccAlloydbUser_alloydbUserBuiltinExample|TestAccAlloydbUser_alloydbUserIamExample |
Rerun these tests in REPLAYING mode to catch issues
|
The failing test is due to an issue with the AlloyDB Control Plane. The fix has been submitted and is waiting to be deployed. I'd appreciate an early feedback about the PR beforehand. |
@pattukerman Thanks for the heads up. I'll start working on the review but will hold merging until the upstream fix. |
Removing the test for now, since AlloyDB control plane deployment for the fix is taking time. The fix is about returning the database roles for a given user in sorted order, so it is not a breaking bug. I will re-add the test once the Control Plane deployment is done.
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. Terraform GA: Diff ( 5 files changed, 971 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataSourceGoogleServiceAccountIdToken_impersonation |
Rerun these tests in REPLAYING mode to catch issues
|
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.
First pass. will look into tests closely after
mmv1/products/alloydb/User.yaml
Outdated
create_url: '{{cluster}}/users?userId={{user_id}}' | ||
update_url: '{{cluster}}/users?userId={{user_id}}' | ||
update_verb: :POST | ||
update_mask: false |
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.
If we're not using udpate mask for this resource, we can omit this field. The default is false
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.
Sounds good, I'll remove the line.
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. Terraform GA: Diff ( 5 files changed, 971 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests:
|
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. Terraform GA: Diff ( 5 files changed, 971 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests:
|
Hey Shuya, |
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.
Some comments + questions. Sorry for the delay on reviewing it.
Add back the update test for IAM user.
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. Terraform GA: Diff ( 5 files changed, 1054 insertions(+), 2 deletions(-)) |
|
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. Terraform GA: Diff ( 5 files changed, 1054 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 9 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbUser_updateRoles_IAM|TestAccAlloydbUser_alloydbUserBuiltinExample|TestAccAlloydbUser_alloydbUserIamExample|TestAccAlloydbUser_updateRoles_BuiltIn|TestAccAlloydbUser_updatePassword_BuiltIn|TestAccContainerCluster_withAddons|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withKubeletConfig|TestAccSpannerInstanceIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
/gcbrun |
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. Terraform GA: Diff ( 5 files changed, 1054 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbUser_updatePassword_BuiltIn|TestAccContainerNodePool_withKubeletConfig|TestAccContainerCluster_withAddons|TestAccContainerNodePool_withUpgradeSettings|TestAccHealthcareDatasetIamPolicy|TestAccDataSourceGoogleServiceAccountJwt |
Rerun these tests in REPLAYING mode to catch issues
|
/gcbrun |
Hey Shuya, |
Thanks for taking a look at the PR. Yes, the failing tests are unrelated to the change of this PR and we don't need to worry about them. I'm giving a final pass now. I'll approve and merge the PR when it's ready to go |
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. Terraform GA: Diff ( 5 files changed, 1049 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbUser_updateRoles_BuiltIn|TestAccAlloydbUser_alloydbUserBuiltinExample|TestAccAlloydbUser_updateRoles_IAM|TestAccAlloydbUser_alloydbUserIamExample|TestAccAlloydbUser_updatePassword_BuiltIn|TestAccWorkstationsWorkstationConfig_update |
Rerun these tests in REPLAYING mode to catch issues
|
/gcbrun |
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. Terraform GA: Diff ( 5 files changed, 1049 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbUser_updateRoles_IAM |
Rerun these tests in REPLAYING mode to catch issues
|
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! Thank you!
AlloyDB hosts different resources in its public API, including Clusters, Instances, and Backups. All those have their corresponding Terraform integration. One new AlloyDB resource that got published in 2023 April is AlloyDB Users resource and API. This PR introduces support for it.
Release Note Template for Downstream PRs (will be copied)