chore(secretmanager): Add regional samples for delayed destory #13317
Conversation
|
Here is the summary of changes. You are about to add 3 region tags.
This comment is generated by snippet-bot.
|
product-auto-label
bot
added
api: secretmanager
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @archana-9430, 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 regional samples for delayed destroy functionality in the Secret Manager. It includes new files for creating, disabling, and updating secrets with delayed destroy, along with corresponding tests. The changes primarily involve creating new functions and tests to manage the version destroy TTL for secrets in specific regions.
Highlights
- New Samples: Adds new samples for creating regional secrets with delayed destroy, disabling delayed destroy, and updating secrets with delayed destroy.
- Test Cases: Includes new test cases to validate the functionality of creating, disabling, and updating secrets with delayed destroy.
- Regional Focus: Ensures that all operations are performed within a specified region by using regional endpoints.
Changelog
Click here to see the changelog
- secretmanager/snippets/regional_samples/create_regional_secret_with_delayed_destroy.py
- Added a new sample to create a regional secret with a specified version destroy TTL.
- Includes argument parsing for project ID, location ID, secret ID, and version destroy TTL.
- Uses the Secret Manager client to create the secret with the specified TTL.
- secretmanager/snippets/regional_samples/disable_regional_secret_delayed_destroy.py
- Added a new sample to disable delayed destroy on an existing regional secret.
- Includes argument parsing for project ID, location ID, and secret ID.
- Uses the Secret Manager client to update the secret and remove the version destroy TTL.
- secretmanager/snippets/regional_samples/snippets_test.py
- Added new test cases for creating, disabling, and updating secrets with delayed destroy.
- Includes fixtures for creating secrets with delayed destroy for testing purposes.
- Imports new functions for delayed destroy operations.
- Added a new fixture
version_destroy_ttlto define the version destroy ttl to 7 days in seconds.
- secretmanager/snippets/regional_samples/update_regional_secret_with_delayed_destroy.py
- Added a new sample to update the version destroy TTL on an existing regional secret.
- Includes argument parsing for project ID, location ID, secret ID, and new version destroy TTL.
- Uses the Secret Manager client to update the secret with the new TTL.
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 regions far, secrets reside,
With TTLs, their fate to decide.
Create, disable, update with care,
Lest your secrets vanish in air.
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.
Code Review
This pull request introduces new samples for creating, disabling, and updating secrets with delayed destroy functionality in the Secret Manager API. The changes include new Python scripts and corresponding tests. Overall, the code is well-structured and addresses the intended functionality. However, there are a few areas that could be improved for clarity and consistency.
Summary of Findings
- Inconsistent Argument Naming: The argument name
version_destroy_ttlis used in some functions whilenew_version_destroy_ttlis used in others. Consistency in naming would improve readability. - Missing Input Validation: The code does not validate the
version_destroy_ttlinput. It should check if the value is within the allowed range (e.g., positive and not exceeding the maximum allowed TTL). - Duplicated API Endpoint Definition: The API endpoint is defined in each function. It would be better to define it once as a constant or pass it as an argument to avoid duplication.
Merge Readiness
The pull request introduces important functionality for managing secrets with delayed destroy. However, addressing the identified issues related to argument naming consistency, input validation, and API endpoint definition would improve the code's robustness and maintainability. I am unable to approve this pull request, and recommend that another reviewer approves this code before merging. At a minimum, the high severity issues should be addressed before merging.
| parser.add_argument( | ||
| "version_destroy_ttl", help="version_destroy_ttl you want to add" | ||
| ) |
There was a problem hiding this comment.
The version_destroy_ttl argument is defined but not used in the disable_regional_secret_delayed_destroy function. Remove it to avoid confusion.
parser.add_argument("secret_id", help="id of the secret to act on")
args = parser.parse_args()
disable_regional_secret_delayed_destroy(
args.project_id, args.location_id, args.secret_id
)| ) -> secretmanager_v1.UpdateSecretRequest: | ||
| """ |
There was a problem hiding this comment.
The return type annotation is incorrect. The function returns a secretmanager_v1.Secret object, not a secretmanager_v1.UpdateSecretRequest object. Fixing this will improve code clarity and prevent potential type-related issues.
def update_regional_secret_with_delayed_destroy(
project_id: str, location_id: str, secret_id: str, new_version_destroy_ttl: int
) -> secretmanager_v1.Secret:| "version_destroy_ttl", help="version_destroy_ttl you want to add" | ||
| ) |
There was a problem hiding this comment.
The version_destroy_ttl argument is passed as a string from the command line, but it's used as an integer in the create_regional_secret_with_delayed_destroy function. Consider converting it to an integer here to avoid potential type errors.
| "version_destroy_ttl", help="version_destroy_ttl you want to add" | |
| ) | |
| "version_destroy_ttl", help="version_destroy_ttl you want to add" | |
| ) | |
| args = parser.parse_args() | |
| create_regional_secret_with_delayed_destroy( | |
| args.project_id, args.location_id, args.secret_id, int(args.version_destroy_ttl) | |
| ) |
| project_id: str, location_id: str, secret_id: str, new_version_destroy_ttl: int | ||
| ) -> secretmanager_v1.UpdateSecretRequest: |
There was a problem hiding this comment.
The function signature uses new_version_destroy_ttl while the argument in main is still version_destroy_ttl. For consistency, use new_version_destroy_ttl in main as well.
def update_regional_secret_with_delayed_destroy(
project_id: str, location_id: str, secret_id: str, new_version_destroy_ttl: int
) -> secretmanager_v1.UpdateSecretRequest:| "version_destroy_ttl", help="version_destroy_ttl you want to add" | ||
| ) |
There was a problem hiding this comment.
The argument name version_destroy_ttl does not match the function parameter new_version_destroy_ttl. Rename it to new_version_destroy_ttl for consistency.
"new_version_destroy_ttl", help="version_destroy_ttl you want to add"
)
args = parser.parse_args()
update_regional_secret_with_delayed_destroy(
args.project_id, args.location_id, args.secret_id, args.new_version_destroy_ttl
)
Description
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
nox -s py-3.9(see Test Environment Setup)nox -s lint(see Test Environment Setup)