-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(bigquery): Add samples for control access 3/3 #4025
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 viewing access policies for BigQuery datasets, tables, or views. It includes new functions viewDatasetAccessPolicy and viewTableOrViewAccessPolicy along with corresponding tests. This is the third pull request in a series related to controlling access, and it depends on the merging of #4023 and #4024.
Highlights
- New samples: Adds samples for viewing access policies for BigQuery datasets, tables, and views, as per the documentation links provided.
- New functions: Introduces
viewDatasetAccessPolicyandviewTableOrViewAccessPolicyto retrieve and display access policies. - Testing: Includes new tests to verify the functionality of the added samples.
Changelog
Click here to see the changelog
- bigquery/cloud-client/test/viewDatasetAccessPolicy.test.js
- Added a new test file to verify the functionality of
viewDatasetAccessPolicy. - The test checks if the access policy is defined and is an array.
- Added a new test file to verify the functionality of
- bigquery/cloud-client/test/viewTableOrViewAccessPolicy.test.js
- Added a new test file to verify the functionality of
viewTableOrViewAccessPolicy. - The tests check if the policy exists, bindings are an array (and empty), and etag is defined for both tables and views.
- Added a new test file to verify the functionality of
- bigquery/cloud-client/viewDatasetAccessPolicy.js
- Added a new function
viewDatasetAccessPolicyto retrieve and display access policies for a given dataset ID. - The function retrieves dataset metadata and logs access entries, including role, special group, and user by email.
- Added a new function
- bigquery/cloud-client/viewTableOrViewAccessPolicy.js
- Added a new function
viewTableOrViewAccessPolicyto retrieve and display access policies for a BigQuery table or view. - The function retrieves the IAM policy for the table or view and logs policy details, including bindings, etag, and version.
- Added a new function
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.
In BigQuery's realm, access we seek,
Policies to view, both strong and meek.
With datasets and tables, a careful eye,
Ensuring that only the right can spy.
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 NodeJS samples for viewing access policies for BigQuery datasets, tables, and views. The code includes tests and implementation for these functionalities. Overall, the code seems well-structured and follows the documentation links provided. However, there are a few areas that could be improved for clarity and completeness.
Summary of Findings
- TODO comments: The code contains
TODOcomments that should be addressed before merging. These comments indicate areas where the developer needs to update or uncomment lines for the code to function correctly in a real-world scenario. - Incomplete error handling: The code lacks proper error handling for scenarios where the dataset, table, or view might not exist. This could lead to unhandled exceptions and unexpected behavior.
- Missing input validation: The code does not validate the input parameters (projectId, datasetId, resourceName) to ensure they are valid. This could lead to errors if the parameters are incorrect or missing.
Merge Readiness
The pull request is not ready to be merged due to the presence of TODO comments and missing error handling. These issues need to be addressed to ensure the code functions correctly and is robust enough for production use. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. At a minimum, the TODO comments should be addressed and proper error handling should be implemented before merging.
| // We're not checking the exact number of entries since that might vary, | ||
| // but we're making sure the appropriate logging format was followed | ||
| assert.ok( | ||
| console.log.calledWith(sinon.match(/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.
praise: nice use of match() here.
|
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 3/3
Merge this PR once #4023 and #4024 have been merged.
Checklist
npm test(see Testing)