chore(secretmanager): Add global samples for delayed destory #13312
Conversation
|
Here is the summary of changes. You are about to add 3 region tags.
This comment is generated by snippet-bot.
|
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 new samples for delayed destroy functionality in the Secret Manager. It includes code to create a secret with a delayed destroy TTL, disable the delayed destroy, and update the secret with a new delayed destroy TTL. The PR also includes corresponding tests for these new functionalities.
Highlights
- New Samples: Adds
create_secret_with_delayed_destroy.py,disable_secret_with_delayed_destroy.py, andupdate_secret_with_delayed_destroy.pyto demonstrate the delayed destroy feature. - Test Coverage: Extends
snippets_test.pyto include tests for the new delayed destroy samples, ensuring proper functionality. - Functionality: Adds functionality to create, disable, and update secrets with delayed destroy.
Changelog
Click here to see the changelog
- secretmanager/snippets/create_secret_with_delayed_destroy.py
- Adds a new sample to create a secret with a specified version destroy TTL.
- Includes argument parsing for project ID, secret ID, and version destroy TTL.
- Uses the Secret Manager client to create the secret with the delayed destroy setting.
- secretmanager/snippets/disable_secret_with_delayed_destroy.py
- Adds a new sample to disable the delayed destroy setting on a secret.
- Includes argument parsing for project ID and secret ID.
- Uses the Secret Manager client to update the secret and remove the version destroy TTL.
- secretmanager/snippets/snippets_test.py
- Imports new modules for delayed destroy functionality.
- Adds a new fixture
secret_with_delayed_destroyto create a secret with delayed destroy for testing. - Adds tests for creating and disabling secrets with delayed destroy.
- Adds a test for updating secrets with delayed destroy.
- secretmanager/snippets/update_secret_with_delayed_destroy.py
- Adds a new sample to update the version destroy TTL on an existing secret.
- Includes argument parsing for project ID, secret ID, and the new version destroy TTL.
- Uses the Secret Manager client to update the secret with the new delayed destroy setting.
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.
A secret's life,
A TTL to decide,
When it must fade.
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
The pull request introduces new functionalities for creating, disabling, and updating secrets with delayed destroy options in the Secret Manager API. The changes include new sample snippets 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 correctness.
Summary of Findings
- Inconsistent Argument Naming: The argument name
version_destroy_ttlis used in multiple functions, but inupdate_secret_with_delayed_destroy.py, it's namednew_version_destroy_ttl. Consistent naming improves readability and reduces confusion. - Incorrect Assertion in Test: In
test_update_secret_with_delayed_destroy, the assertionassert updated_secret.version_destroy_ttl == timedelta(seconds=version_destroy_ttl)compares against the originalversion_destroy_ttlfixture value instead of theupdated_version_destroy_ttl_valueused in the update call. This will cause the test to fail. - Missing Input Validation: The code does not validate the
version_destroy_ttlinput. It should check if the value is within the allowed range and is a valid integer.
Merge Readiness
The pull request is almost ready for merging, but there are a few issues that need to be addressed. Specifically, the inconsistent argument naming, incorrect assertion in the test, and missing input validation should be fixed before merging. I am unable to approve this pull request, and other reviewers should review and approve this code before merging.
| updated_secret = update_secret_with_delayed_destroy(project_id, secret_id, updated_version_destroy_ttl_value) | ||
| assert updated_secret.version_destroy_ttl == timedelta(seconds=version_destroy_ttl) |
There was a problem hiding this comment.
The assertion compares against the original version_destroy_ttl fixture value instead of the updated_version_destroy_ttl_value used in the update call. This will cause the test to fail. It should be updated to compare against updated_version_destroy_ttl_value.
| updated_secret = update_secret_with_delayed_destroy(project_id, secret_id, updated_version_destroy_ttl_value) | |
| assert updated_secret.version_destroy_ttl == timedelta(seconds=version_destroy_ttl) | |
| updated_secret = update_secret_with_delayed_destroy(project_id, secret_id, updated_version_destroy_ttl_value) | |
| assert updated_secret.version_destroy_ttl == timedelta(seconds=updated_version_destroy_ttl_value) |
| return response | ||
|
|
||
|
|
||
| # [END secretmanager_create_secret_with_delayed_destroy] |
There was a problem hiding this comment.
nit: remove the trailing empty lines
| from google.protobuf.duration_pb2 import Duration | ||
|
|
||
|
|
||
| # [START secretmanager_update_secret_with_delayed_destroy] |
There was a problem hiding this comment.
fix: region tag should start before all necessary imports (line 17 in this case)
|
Hi Remigiusz, |
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)