-
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
docs(iam): Update comments and terminology in IAM samples #9887
base: main
Are you sure you want to change the base?
Conversation
Includes changing "member" to "principal" and linking to principal identifiers page instead of specifying recommended format.
Includes changing "member" to "principal" and linking to principal identifiers page instead of specifying recommended format.
Includes changing "member" to "principal" and linking to principal identifiers page instead of specifying recommended format. melaniedejong-patch-1 (#4895)
Includes changing "member" to "principal" and linking to principal identifiers page instead of specifying recommended format.
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.
Hi @melaniedejong and reviewers,
I'm currently reviewing this pull request and will post my full review in a few minutes. In the meantime, here's a quick summary to help everyone get up to speed:
This pull request aims to improve the IAM samples by updating comments and terminology. Specifically, it replaces the term "member" with "principal" throughout the codebase and provides a link to the principal identifiers documentation instead of hardcoding a recommended format.
Here's a breakdown of the changes:
-
Files Modified:
iam/snippets/src/main/java/AddBinding.java
iam/snippets/src/main/java/AddMember.java
iam/snippets/src/main/java/Quickstart.java
iam/snippets/src/main/java/RemoveMember.java
-
Specific Changes:
- Lines 30-31 in
AddBinding.java
: Updated comments to use "principals" and added a link to the principal identifiers documentation. - Lines 29-30 in
AddMember.java
: Updated comments to use "principal" and added a link to the principal identifiers documentation. - Lines 47-48 in
AddMember.java
: Changed the comment to reflect that a principal is being added. - Lines 37-38 in
Quickstart.java
: Updated comments to use "principal" and added a link to the principal identifiers documentation. - Lines 59-60 and 73-74 in
Quickstart.java
: Changed comments to use "principal" instead of "member". - Lines 107-108 in
Quickstart.java
: Changed comment to reflect adding a principal instead of a member. - Lines 127-128 in
Quickstart.java
: Changed comment to reflect removing a principal instead of a member. - Lines 29-30 in
RemoveMember.java
: Updated comments to use "principal" and added a link to the principal identifiers documentation. - Lines 52-53 and 70-71 in
RemoveMember.java
: Changed comments to reflect removing and updating a principal instead of a member.
- Lines 30-31 in
The changes are straightforward and improve clarity and consistency in the IAM samples. The use of "principal" is more accurate and the link to the documentation is helpful for users.
And now, a little haiku to lighten the mood:
Code flows like a stream,
Reviews guide it to the sea,
Perfect, polished code.
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 pull request updates terminology and comments in the IAM samples, replacing "member" with "principal" and linking to the principal identifiers page. This improves clarity and consistency with IAM documentation.
I've reviewed the changes according to the Google Java Style Guide and found them to be well-implemented. The changes correctly update all instances of "member" to "principal" in comments and strings, and provide helpful links to the documentation. The updated TODO comments are also clear and informative.
One area for improvement is consistency in variable naming. While the comments and output messages have been updated to use "principal," the variable names themselves still use "member." This could lead to confusion. I recommend renaming these variables for better consistency and readability. I've included specific suggestions in the file reviews below.
// TODO: Replace with your principals. | ||
// For examples, see https://cloud.google.com/iam/docs/principal-identifiers | ||
List<String> members = Collections.singletonList("principal-id"); |
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.
For better consistency with the updated terminology, consider renaming the members
variable to principals
. This aligns the variable name with the concept of principals and makes the code easier to understand. This change adheres to the Google Java Style Guide's recommendation for clear and descriptive variable names.
// TODO: Replace with your principals. | |
// For examples, see https://cloud.google.com/iam/docs/principal-identifiers | |
List<String> members = Collections.singletonList("principal-id"); | |
List<String> principals = Collections.singletonList("principal-id"); |
// TODO: Replace with your principal. | ||
// For examples, see https://cloud.google.com/iam/docs/principal-identifiers | ||
String member = "principal-id"; |
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.
Consider renaming the member
variable to principal
for consistency with the updated terminology. This aligns the variable name with the concept of principals and makes the code easier to understand. This change adheres to the Google Java Style Guide's recommendation for clear and descriptive variable names.
// TODO: Replace with your principal. | |
// For examples, see https://cloud.google.com/iam/docs/principal-identifiers | |
String member = "principal-id"; | |
String principal = "principal-id"; |
// TODO: Replace with the ID of your principal. | ||
// For examples, see https://cloud.google.com/iam/docs/principal-identifiers | ||
String member = "your-principal"; |
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.
Consider renaming the member
variable to principal
for consistency with the updated terminology. This aligns the variable name with the concept of principals and makes the code easier to understand. This change adheres to the Google Java Style Guide's recommendation for clear and descriptive variable names.
// TODO: Replace with the ID of your principal. | |
// For examples, see https://cloud.google.com/iam/docs/principal-identifiers | |
String member = "your-principal"; | |
String principal = "your-principal"; |
// TODO: Replace with your principal. | ||
// For examples, see https://cloud.google.com/iam/docs/principal-identifiers | ||
String member = "principal-id"; |
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.
Consider renaming the member
variable to principal
for consistency with the updated terminology. This aligns the variable name with the concept of principals and makes the code easier to understand. This change adheres to the Google Java Style Guide's recommendation for clear and descriptive variable names.
// TODO: Replace with your principal. | |
// For examples, see https://cloud.google.com/iam/docs/principal-identifiers | |
String member = "principal-id"; | |
String principal = "principal-id"; |
I'm unsure why Kokoro is failing. From the logs, it seems like it's getting a null value for the environment variable 'IAM_CREDENTIALS' during the deny integration tests:
But I don't think that the updates I made would affect that variable. Could a reviewer help me troubleshoot? |
Description
Includes changing "member" to "principal" and linking to principal identifiers page instead of specifying recommended format.
I left "member" when it referred to the field in the role binding, since that field is still called "members."
Please merge this PR for me once it is approved