-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat: Adding disable role snippet #9295
Conversation
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
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 snippet!
Few minor comments.
UpdateRoleRequest updateRoleRequest = | ||
UpdateRoleRequest.newBuilder() | ||
.setName(roleName) | ||
.setRole(roleBuilder) |
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.
Instead of the Role.Builder, can we use Role?
Role.Builder roleBuilder = | ||
Role.newBuilder() | ||
.setName(roleName) | ||
.setStage(Role.RoleLaunchStage.DISABLED); |
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.
Can we build this and get back the Role, like below?
Role.Builder roleBuilder = | |
Role.newBuilder() | |
.setName(roleName) | |
.setStage(Role.RoleLaunchStage.DISABLED); | |
Role role = | |
Role.newBuilder() | |
.setName(roleName) | |
.setStage(Role.RoleLaunchStage.DISABLED) | |
.build(); |
// once, and can be reused for multiple requests. | ||
try (IAMClient iamClient = IAMClient.create()) { | ||
Role result = iamClient.updateRole(updateRoleRequest); | ||
System.out.println("Disabled role:\n" + result); |
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.
Kindly return the updated role to be used in test assert statements.
@@ -111,6 +111,10 @@ public void testRole() throws IOException { | |||
ListRoles.listRoles(projectId); | |||
assertThat(bout.toString().contains(roleId)); | |||
|
|||
// Test disable role. | |||
DisableRole.disableRole(projectId, roleId); | |||
assertThat(bout.toString().contains("Stage: DISABLED")); |
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.
Instead of asserting from sysout, it's more robust to retrieve and verify the status from the updated role.
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
@msilc PTAL at the lint issues |
* Added the disable role snippet * Refactoring * Updated header * PR fix * Checkstyle fix * Names reverted
* Added the disable role snippet * Refactoring * Updated header * PR fix * Checkstyle fix * Names reverted
Description
Added a new disable role snippet
Checklist
pom.xml
parent set to latestshared-configuration
mvn clean verify
requiredmvn -P lint checkstyle:check
requiredmvn -P lint clean compile pmd:cpd-check spotbugs:check
advisory only