feat(secretmanager): Adding expiretime samples #4217
Conversation
…odejs-docs-samples into node_list_samples
…odejs-docs-samples into node_cmek_samples
product-auto-label
bot
added
api: secretmanager
|
Here is the summary of changes. You are about to add 17 region tags.
This comment is generated by snippet-bot.
|
Summary of ChangesHello @khilan-crest, 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! This pull request significantly enhances the Google Cloud Secret Manager Node.js samples by introducing a suite of new functionalities. The additions focus on advanced secret management features, including the ability to define and manage secret expiration, utilize customer-managed encryption keys, and configure custom replication policies. Furthermore, the PR provides new ways to query and organize secrets through filtering and tag binding management. These updates aim to provide developers with clearer examples and broader coverage of the Secret Manager API's capabilities. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds several new samples for Secret Manager, demonstrating features like expiration times, CMEK, user-managed replication, and tag management. The changes are comprehensive and cover both global and regional secrets.
My review has found a few issues:
- A recurring critical bug where asynchronous operations are not awaited, which would cause scripts to terminate prematurely.
- A bug in one sample where a parameter is passed with an incorrect type.
- Some opportunities for code simplification by removing unnecessarily nested functions.
- A minor inconsistency in code formatting.
I've provided specific suggestions to address these points in the comments below. Once these are addressed, the new samples will be much more robust and easier to understand.
| async function deleteRegionalSecretExpiration() { | ||
| // Update the secret to remove the expiration time | ||
| const [secret] = await client.updateSecret({ | ||
| secret: { | ||
| name: name, | ||
| // No expireTime field specified, which will clear it | ||
| }, | ||
| updateMask: { | ||
| paths: ['expire_time'], | ||
| }, | ||
| }); | ||
|
|
||
| console.log(`Removed expiration from secret ${secret.name}`); | ||
| } | ||
|
|
||
| deleteRegionalSecretExpiration(); |
There was a problem hiding this comment.
The main function is async, but the call to deleteRegionalSecretExpiration() is not awaited. This can lead to the script exiting before the asynchronous operation completes.
Additionally, the deleteRegionalSecretExpiration function is defined and then immediately called only once. You can simplify the code by removing this inner function and moving its logic directly into the main function. This improves readability and fixes the bug.
// Update the secret to remove the expiration time
const [secret] = await client.updateSecret({
secret: {
name: name,
// No expireTime field specified, which will clear it
},
updateMask: {
paths: ['expire_time'],
},
});
console.log(`Removed expiration from secret ${secret.name}`);There was a problem hiding this comment.
All samples contains inner function within main which is showing main functionality of the sample. For consistency with other samples this is not required.
| async function createRegionalSecretWithExpiration() { | ||
| // Calculate expiration time (1 hour from now) | ||
| const expireTime = new Date(); | ||
| expireTime.setHours(expireTime.getHours() + 1); | ||
|
|
||
| // Create the secret with automatic replication and expiration time | ||
| const [secret] = await client.createSecret({ | ||
| parent: parent, | ||
| secretId: secretId, | ||
| secret: { | ||
| expireTime: { | ||
| seconds: Math.floor(expireTime.getTime() / 1000), | ||
| nanos: (expireTime.getTime() % 1000) * 1000000, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| console.log( | ||
| `Created secret ${secret.name} with expiration time ${expireTime.toISOString()}` | ||
| ); | ||
| } | ||
|
|
||
| createRegionalSecretWithExpiration(); |
There was a problem hiding this comment.
The main function is async, but the call to createRegionalSecretWithExpiration() is not awaited. This can lead to the script exiting before the asynchronous operation completes.
Additionally, the createRegionalSecretWithExpiration function is defined and then immediately called only once. You can simplify the code by removing this inner function and moving its logic directly into the main function. This improves readability and fixes the bug.
// Calculate expiration time (1 hour from now)
const expireTime = new Date();
expireTime.setHours(expireTime.getHours() + 1);
// Create the secret with automatic replication and expiration time
const [secret] = await client.createSecret({
parent: parent,
secretId: secretId,
secret: {
expireTime: {
seconds: Math.floor(expireTime.getTime() / 1000),
nanos: (expireTime.getTime() % 1000) * 1000000,
},
},
});
console.log(
`Created secret ${secret.name} with expiration time ${expireTime.toISOString()}`
);There was a problem hiding this comment.
All samples contains inner function within main which is showing main functionality of the sample. For consistency with other samples this is not required.
| async function updateSecretExpiration() { | ||
| // Calculate new expiration time (2 hours from now) | ||
| const newExpireTime = new Date(); | ||
| newExpireTime.setHours(newExpireTime.getHours() + 2); | ||
|
|
||
| // Update the expire_time | ||
| const [response] = await client.updateSecret({ | ||
| secret: { | ||
| name: name, | ||
| expireTime: { | ||
| seconds: Math.floor(newExpireTime.getTime() / 1000), | ||
| nanos: (newExpireTime.getTime() % 1000) * 1000000, | ||
| }, | ||
| }, | ||
| updateMask: { | ||
| paths: ['expire_time'], | ||
| }, | ||
| }); | ||
|
|
||
| // Print the updated secret name | ||
| console.log( | ||
| `Updated secret ${response.name} expiration time to ${newExpireTime.toISOString()}` | ||
| ); | ||
| } | ||
|
|
||
| updateSecretExpiration(); |
There was a problem hiding this comment.
The main function is async, but the call to updateSecretExpiration() is not awaited. This can lead to the script exiting before the asynchronous operation completes.
Additionally, the updateSecretExpiration function is defined and then immediately called only once. You can simplify the code by removing this inner function and moving its logic directly into the main function. This improves readability and fixes the bug.
// Calculate new expiration time (2 hours from now)
const newExpireTime = new Date();
newExpireTime.setHours(newExpireTime.getHours() + 2);
// Update the expire_time
const [response] = await client.updateSecret({
secret: {
name: name,
expireTime: {
seconds: Math.floor(newExpireTime.getTime() / 1000),
nanos: (newExpireTime.getTime() % 1000) * 1000000,
},
},
updateMask: {
paths: ['expire_time'],
},
});
// Print the updated secret name
console.log(
`Updated secret ${response.name} expiration time to ${newExpireTime.toISOString()}`
);There was a problem hiding this comment.
All samples contains inner function within main which is showing main functionality of the sample. For consistency with other samples this is not required.
| async function createUmmrSecret() { | ||
| // Create the secret configuration | ||
| const secretConfig = { | ||
| replication: { | ||
| userManaged: { | ||
| replicas: locations.map(location => ({location})), | ||
| }, | ||
| }, | ||
| ttl: { | ||
| seconds: ttl, | ||
| }, | ||
| }; | ||
|
|
||
| // Create the secret | ||
| const [secret] = await client.createSecret({ | ||
| parent: parent, | ||
| secretId: secretId, | ||
| secret: secretConfig, | ||
| }); | ||
|
|
||
| console.log(`Created secret: ${secret.name}`); | ||
| } | ||
|
|
||
| createUmmrSecret(); |
There was a problem hiding this comment.
The main function is async, but the call to createUmmrSecret() is not awaited. This can lead to the script exiting before the asynchronous operation completes.
Additionally, the createUmmrSecret function is defined and then immediately called only once. You can simplify the code by removing this inner function and moving its logic directly into the main function. This improves readability and fixes the bug.
// Create the secret configuration
const secretConfig = {
replication: {
userManaged: {
replicas: locations.map(location => ({location})),
},
},
ttl: {
seconds: ttl,
},
};
// Create the secret
const [secret] = await client.createSecret({
parent: parent,
secretId: secretId,
secret: secretConfig,
});
console.log(`Created secret: ${secret.name}`);There was a problem hiding this comment.
All samples contains inner function within main which is showing main functionality of the sample. For consistency with other samples this is not required.
| async function updateRegionalSecretExpiration() { | ||
| // Calculate new expiration time (2 hours from now) | ||
| const newExpireTime = new Date(); | ||
| newExpireTime.setHours(newExpireTime.getHours() + 2); | ||
|
|
||
| // Update the secret with the new expiration time | ||
| const [secret] = await client.updateSecret({ | ||
| secret: { | ||
| name: name, | ||
| expireTime: { | ||
| seconds: Math.floor(newExpireTime.getTime() / 1000), | ||
| nanos: (newExpireTime.getTime() % 1000) * 1000000, | ||
| }, | ||
| }, | ||
| updateMask: { | ||
| paths: ['expire_time'], | ||
| }, | ||
| }); | ||
|
|
||
| console.log( | ||
| `Updated secret ${secret.name} expiration time to ${newExpireTime.toISOString()}` | ||
| ); | ||
| } | ||
|
|
||
| updateRegionalSecretExpiration(); |
There was a problem hiding this comment.
The main function is async, but the call to updateRegionalSecretExpiration() is not awaited. This can lead to the script exiting before the asynchronous operation completes.
Additionally, the updateRegionalSecretExpiration function is defined and then immediately called only once. You can simplify the code by removing this inner function and moving its logic directly into the main function. This improves readability and fixes the bug.
// Calculate new expiration time (2 hours from now)
const newExpireTime = new Date();
newExpireTime.setHours(newExpireTime.getHours() + 2);
// Update the secret with the new expiration time
const [secret] = await client.updateSecret({
secret: {
name: name,
expireTime: {
seconds: Math.floor(newExpireTime.getTime() / 1000),
nanos: (newExpireTime.getTime() % 1000) * 1000000,
},
},
updateMask: {
paths: ['expire_time'],
},
});
console.log(
`Updated secret ${secret.name} expiration time to ${newExpireTime.toISOString()}`
);There was a problem hiding this comment.
All samples contains inner function within main which is showing main functionality of the sample. For consistency with other samples this is not required.
| async function listRegionalSecretVersionsWithFilter() { | ||
| const [versions] = await client.listSecretVersions({ | ||
| parent: parent, | ||
| filter: filterStr, | ||
| }); | ||
|
|
||
| versions.forEach(version => { | ||
| console.log(`Found version: ${version.name}`); | ||
| }); | ||
| } | ||
|
|
||
| listRegionalSecretVersionsWithFilter(); |
There was a problem hiding this comment.
The main function is async, but the call to listRegionalSecretVersionsWithFilter() is not awaited. This can lead to the script exiting before the asynchronous operation completes.
Additionally, the listRegionalSecretVersionsWithFilter function is defined and then immediately called only once. You can simplify the code by removing this inner function and moving its logic directly into the main function. This improves readability and fixes the bug.
const [versions] = await client.listSecretVersions({
parent: parent,
filter: filterStr,
});
versions.forEach(version => {
console.log(`Found version: ${version.name}`);
});There was a problem hiding this comment.
All samples contains inner function within main which is showing main functionality of the sample. For consistency with other samples this is not required.
| async function listRegionalSecretsWithFilter() { | ||
| const [secrets] = await client.listSecrets({ | ||
| parent: parent, | ||
| filter: filterStr, | ||
| }); | ||
|
|
||
| secrets.forEach(secret => { | ||
| console.log(`Found secret: ${secret.name}`); | ||
| }); | ||
| } | ||
|
|
||
| listRegionalSecretsWithFilter(); |
There was a problem hiding this comment.
The main function is async, but the call to listRegionalSecretsWithFilter() is not awaited. This can lead to the script exiting before the asynchronous operation completes.
Additionally, the listRegionalSecretsWithFilter function is defined and then immediately called only once. You can simplify the code by removing this inner function and moving its logic directly into the main function. This improves readability and fixes the bug.
const [secrets] = await client.listSecrets({
parent: parent,
filter: filterStr,
});
secrets.forEach(secret => {
console.log(`Found secret: ${secret.name}`);
});There was a problem hiding this comment.
All samples contains inner function within main which is showing main functionality of the sample. For consistency with other samples this is not required.
| }, | ||
| }, | ||
| ttl: { | ||
| seconds: ttl, |
There was a problem hiding this comment.
The createSecret API expects the seconds field for ttl to be a number. The current code passes the ttl string argument directly. Based on the example in the comment ('7776000s') and the test, this will be a string with an 's' suffix, which will cause an error. You should parse the integer value from the ttl argument.
seconds: parseInt(ttl, 10),There was a problem hiding this comment.
Done
Updated code to pass int not str
| } | ||
| // [END secretmanager_create_regional_secret_with_expire_time] |
There was a problem hiding this comment.
For consistency with other samples, the // [END ...] region tag should be placed inside the main function, just before its closing brace.
// [END secretmanager_create_regional_secret_with_expire_time]
}| } | ||
| // [END secretmanager_create_secret_with_expiration] |
There was a problem hiding this comment.
For consistency with other samples, the // [END ...] region tag should be placed inside the main function, just before its closing brace.
// [END secretmanager_create_secret_with_expiration]
}1 participant
Adding expiretime samples
Fixes #
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.