-
Notifications
You must be signed in to change notification settings - Fork 593
HDDS-13887. [STS] Protobuf Plumbing for AssumeRole Requests #9254
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
Conversation
sodonnel
left a comment
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 change LGTM. Its basically all boiler plate code to wire up the new request and protobuf stuff.
If CI passes and you are happy with fields etc we can commit onto the branch.
| public AssumeRoleResponseInfo assumeRole(String roleArn, | ||
| String roleSessionName, | ||
| int durationSeconds, | ||
| String awsIamSessionPolicy) throws IOException { |
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.
Please do not format method signatures like this. Whenever visibility / return type / method name / other modifiers are changed, we would have to reindent all parameters.
(The same comment applies to other files.)
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.
The style guide says you should indent 4 spaces rather than align at the bracket for this reason. Many method definitions in Ozone fill the line length with parameters on the same line, which is less readable IMO.
I would be nice if checkstyle could enforce this, but I suspect there are a lot of violations across the code base if we turned it on!
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.
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.
updated
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!
Please describe your PR in detail:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13887
How was this patch tested?
Unit tests