chore(secretmanager): add global samples for delayed destroy #4073
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 @YashSaraf11, 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 the Secret Manager service related to delayed destroy functionality. It includes samples for creating a secret with delayed destroy enabled, disabling delayed destroy, and updating a secret's delayed destroy settings. The PR also includes corresponding tests for these new samples.
Highlights
- New samples: Adds
createSecretWithDelayedDestroy.js,disableSecretDelayedDestroy.js, andupdateSecretWithDelayedDestroy.jsto demonstrate delayed destroy functionality. - Test coverage: Adds new tests to
secretmanager.test.jsto verify the functionality of the new samples.
Changelog
- secret-manager/createSecretWithDelayedDestroy.js
- Adds a new sample to create a secret with delayed destroy enabled using
version_destroy_ttl.
- Adds a new sample to create a secret with delayed destroy enabled using
- secret-manager/disableSecretDelayedDestroy.js
- Adds a new sample to disable the delayed destroy setting on a secret by updating the
version_destroy_ttl.
- Adds a new sample to disable the delayed destroy setting on a secret by updating the
- secret-manager/test/secretmanager.test.js
- Adds tests for creating, disabling, and updating secrets with delayed destroy functionality.
- secret-manager/updateSecretWithDelayedDestroy.js
- Adds a new sample to update the delayed destroy setting on a secret.
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 time to live, a time to fade,
Destroy, but not yet.
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 samples for creating, disabling, and updating secrets with delayed destroy functionality in the Secret Manager. The changes include new JavaScript files for the samples and corresponding tests. Overall, the code seems well-structured and the tests cover the main functionalities. However, there are a few areas that could be improved for clarity and robustness.
Summary of Findings
- Missing Error Handling: The samples lack explicit error handling for the API calls. While the
.catch(console.error)at the end of themainfunction will catch some errors, it doesn't provide specific feedback or handling within the core logic of the functions. This could make debugging more difficult. - Inconsistent Naming in Tests: In the tests, the secret names are sometimes hardcoded and sometimes dynamically generated. This inconsistency can lead to confusion and potential issues with test cleanup.
- Lack of Input Validation: The samples do not validate the input parameters, such as
timeToLive. This could lead to unexpected behavior if invalid values are provided.
Merge Readiness
The pull request introduces important new functionality with adequate tests. However, addressing the error handling and input validation concerns would significantly improve the robustness and maintainability of the code. I recommend addressing these issues before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.
| seconds: timeToLive, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Consider adding error handling here to catch any potential exceptions during secret creation. This will provide more informative error messages and prevent the program from crashing unexpectedly.
}).catch(err => {
console.error(`Failed to create secret: ${err}`);
throw err; // Re-throw the error to prevent further execution
});| updateMask: { | ||
| paths: ['version_destroy_ttl'], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Consider adding error handling here to catch any potential exceptions during secret updating. This will provide more informative error messages and prevent the program from crashing unexpectedly.
}).catch(err => {
console.error(`Failed to disable delayed destroy: ${err}`);
throw err; // Re-throw the error to prevent further execution
});| updateMask: { | ||
| paths: ['version_destroy_ttl'], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Consider adding error handling here to catch any potential exceptions during secret updating. This will provide more informative error messages and prevent the program from crashing unexpectedly.
}).catch(err => {
console.error(`Failed to update secret: ${err}`);
throw err; // Re-throw the error to prevent further execution
});| `node disableSecretDelayedDestroy.js ${secret.name}-delayedDestroy` | ||
| ); |
There was a problem hiding this comment.
The secret name ${secret.name}-delayedDestroy is hardcoded here. It would be better to generate a unique secret ID for this test to avoid potential conflicts with other tests or existing secrets. Also, consider adding a try-catch block around the deleteSecret call to handle cases where the secret might not exist.
const delayedDestroySecretId = `${secretId}-delayedDestroy-${v4()}`;
await client.createSecret({
parent: `projects/${projectId}`,
secretId: delayedDestroySecretId,
secret: {
replication: {
automatic: {},
},
version_destroy_ttl: {
seconds: 24 * 60 * 60,
},
},
});
const output = execSync(
`node disableSecretDelayedDestroy.js projects/${projectId}/secrets/${delayedDestroySecretId}`
);
assert.match(output, new RegExp('Disabled delayed destroy'));
try {
await client.deleteSecret({
name: `projects/${projectId}/secrets/${delayedDestroySecretId}`,
});
} catch (err) {
console.warn(`Failed to delete secret ${delayedDestroySecretId}: ${err}`);
}| const output = execSync( | ||
| `node updateSecretWithDelayedDestroy.js ${secret.name}-delayedDestroy ${updatedTimeToLive}` |
There was a problem hiding this comment.
The secret name ${secret.name}-delayedDestroy is hardcoded here. It would be better to generate a unique secret ID for this test to avoid potential conflicts with other tests or existing secrets. Also, consider adding a try-catch block around the deleteSecret call to handle cases where the secret might not exist.
const delayedDestroySecretId = `${secretId}-delayedDestroy-${v4()}`;
const updatedTimeToLive = 24 * 60 * 60 * 2;
await client.createSecret({
parent: `projects/${projectId}`,
secretId: delayedDestroySecretId,
secret: {
replication: {
automatic: {},
},
version_destroy_ttl: {
seconds: 24 * 60 * 60,
},
},
});
const output = execSync(
`node updateSecretWithDelayedDestroy.js projects/${projectId}/secrets/${delayedDestroySecretId} ${updatedTimeToLive}`
);
assert.match(output, new RegExp('Updated secret'));
try {
await client.deleteSecret({
name: `projects/${projectId}/secrets/${delayedDestroySecretId}`,
});
} catch (err) {
console.warn(`Failed to delete secret ${delayedDestroySecretId}: ${err}`);
}
Description
Fixes - N/A
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
npm test(see Testing)npm run lint(see Style)GoogleCloudPlatform/nodejs-docs-samples. Not a fork.