-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(bigquery): Add samples for control access 2/3 #4024
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
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.
Hello @hivanalejandro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request adds NodeJS samples for revoking access to BigQuery datasets, tables, or views, as described in the Google Cloud documentation. Specifically, it implements samples for:
- Revoking access to a table or view.
- Revoking access to a dataset.
The changes include two new JavaScript files (revokeDatasetAccess.js and revokeTableOrViewAccess.js) containing the code samples, and two corresponding test files (revokeDatasetAccess.test.js and revokeTableOrViewAccess.test.js) to verify the functionality. The PR is part 2 of 3 in a series, and depends on PR #4023 being merged first.
Highlights
- New samples: Adds
revokeDatasetAccess.jsto demonstrate how to revoke access to a BigQuery dataset for a specified entity. - New samples: Adds
revokeTableOrViewAccess.jsto demonstrate how to revoke access to a BigQuery table or view, including options to remove a specific role or principal. - Testing: Includes corresponding test files (
revokeDatasetAccess.test.jsandrevokeTableOrViewAccess.test.js) to ensure the new samples function as expected.
Changelog
- bigquery/cloud-client/revokeDatasetAccess.js
- Implements the
revokeDatasetAccessfunction to remove access for a given entity from a specified dataset. It retrieves the dataset's access entries, filters out the entry matching the entity ID, and updates the dataset metadata. Includes error handling for precondition failures (read-modify-write conflicts) on line 76.
- Implements the
- bigquery/cloud-client/revokeTableOrViewAccess.js
- Implements the
revokeAccessToTableOrViewfunction to remove access to a table or view. It allows specifying a role or principal to remove. The function retrieves the IAM policy, modifies the bindings to exclude the specified role or principal, and updates the policy. Includes error handling on line 112. - The code on lines 85-98 filters out the principal from each binding, and then filters out bindings with empty members.
- Implements the
- bigquery/cloud-client/test/revokeDatasetAccess.test.js
- Adds a test case to verify that
revokeDatasetAccesscorrectly revokes access to a dataset. The test grants access to a dataset, then revokes it, and asserts that the entity no longer has access.
- Adds a test case to verify that
- bigquery/cloud-client/test/revokeTableOrViewAccess.test.js
- Adds test cases to verify that
revokeTableOrViewAccesscorrectly revokes access to a table or view, both for a specific role and for a specific principal. The tests grant access, then revoke it, and assert that the role/principal is no longer present in the policy.
- Adds test cases to verify that
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Trivia time!
What is IAM in the context of cloud computing?
Click here for the answer
IAM stands for Identity and Access Management. It is a framework of policies and technologies for ensuring that the right users (identity) have appropriate access (access management) to technology resources.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
The pull request introduces new samples for revoking access to BigQuery datasets and tables/views. The code is generally well-structured and includes relevant documentation. However, there are a few areas that could be improved for clarity and robustness.
Summary of Findings
- Error Handling: The error handling in
revokeAccessToTableOrViewcould be improved by providing more context in the error message. Consider including the project ID, dataset ID, and resource name in the error message to facilitate debugging. - Redundant Code: In
revokeDatasetAccess.js, the code iterates through the access entries multiple times. This could be optimized by performing the checks within a single loop. - Missing input validation: The functions
revokeDatasetAccessandrevokeAccessToTableOrViewlack input validation. Adding checks to ensure that the input parameters are not null or empty would improve the robustness of the code.
Merge Readiness
The pull request is almost ready for merging. I recommend addressing the identified issues, particularly the error handling and redundant code, to enhance the code's reliability and maintainability. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
telpirion
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.
Overall, this looks really good. Just needs to clean up that if/else if/else block and this is good to merge!
|
Here is the summary of changes. You are about to add 2 region tags.
This comment is generated by snippet-bot.
|
Description
Add the following NodeJS samples:
Step of Internal: b/394478489
PR 2/3
Merge this PR once #4023 has been merged.
Checklist
npm test(see Testing)