-
Notifications
You must be signed in to change notification settings - Fork 6.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
docs(iam): Update comments and terminology in IAM samples #13010
Conversation
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 detailed review in a few minutes. In the meantime, here's a quick summary to help everyone get up to speed:
This pull request updates the comments and terminology in several IAM samples within the python-docs-samples
repository. The main goal is to improve clarity and consistency by replacing the term "member" with "principal" where appropriate and providing a link to the principal identifiers documentation instead of listing specific formats.
Here's a breakdown of the changes:
- Intent: Modernize terminology and improve documentation clarity regarding IAM principals.
- Changes:
- The term "member" has been replaced with "principal" in several files to reflect best practices and Google Cloud's preferred terminology. This change is consistent throughout the code comments and docstrings.
- Instead of explicitly listing possible formats for principal identifiers, the code now includes a link to the official Google Cloud documentation on principal identifiers. This ensures that the information remains up-to-date and avoids potential inconsistencies.
- Affected files:
iam/cloud-client/snippets/iam_modify_policy_add_role.py
(line 17): Changed parametermember
toprincipal
.iam/cloud-client/snippets/modify_policy_add_member.py
(lines 22-39): Updated docstring to use "principal" and added a link to principal identifiers documentation.iam/cloud-client/snippets/modify_policy_remove_member.py
(lines 22-39): Updated docstring to use "principal" and added a link to principal identifiers documentation.iam/cloud-client/snippets/quickstart.py
(lines 19-46 and 115-121): Updated docstrings and comments to use "principal" and added a link to principal identifiers documentation.
The changes are well-documented and easy to understand. The use of "principal" instead of "member" is a good improvement for clarity and consistency.
And now, a little haiku to lighten the mood:
Code flows like a stream,
Clarity's the guiding star bright,
Reviews make it shine.
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 links in the IAM samples, replacing "member" with "principal" and providing a link to principal identifiers. The changes are well-targeted and improve the clarity of the documentation. I have a few minor suggestions based on the Google Python Style Guide to further enhance the documentation and code style.
Based on the Google Python Style Guide, I'd recommend the following general improvements:
- Docstrings: Ensure all public functions and methods have clear, concise docstrings explaining their purpose, arguments, and return values. Use triple quotes for docstrings, even if they are single-line.
- Line Length: Keep lines under 80 characters to improve readability.
- Blank Lines: Use two blank lines between top-level functions and classes, and one blank line between methods within a class.
- Naming: Use descriptive names for variables and functions. For constants, use all uppercase letters with underscores separating words (e.g., MAX_VALUE).
Hi melaniedejong - Test cases are failing and also please check the AI bot suggestions. Thanks for your work. |
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 testing:
But I don't think that the updates I made would affect that variable. Could you help me troubleshoot? |
Co-authored-by: code-review-assist[bot] <182814678+code-review-assist[bot]@users.noreply.github.com>
@melaniedejong I've had @eapl-gemugami figuring out the path to get your fixes passing tests. We should have this unblocked soon. |
It looks like all the changes in this PR have been covered in #13118. If there are more changes, please create a new PR (make sure you sync your fork!) |
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