feat(secretmanager): Adding cmek samples #4216
Conversation
…odejs-docs-samples into node_list_samples
…odejs-docs-samples into node_cmek_samples
|
Here is the summary of changes. You are about to add 11 region tags.
This comment is generated by snippet-bot.
|
product-auto-label
bot
added
api: secretmanager
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 expands the Google Cloud Secret Manager Node.js samples by introducing a suite of new examples. These additions cover advanced secret creation options, such as using Customer-Managed Encryption Keys (CMEK) and user-managed replication policies, alongside enhanced capabilities for listing secrets and their versions with filters. Furthermore, new samples for managing tag bindings on secrets have been included, providing developers with more robust tools for organizing and securing their sensitive data within Google Cloud. 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, focusing on CMEK, user-managed replication, and filtering/tagging secrets. The changes are well-structured and the new functionality is covered by tests. I've identified a few areas for improvement, including a recurring pattern of redundant nested functions that can be simplified, a potential bug in handling optional arguments, and a minor error in a test assertion. Overall, this is a great addition.
| const output = execSync( | ||
| `node createSecretWithUserManagedReplicationPolicy.js ${parent} ${secretId}-ummr ${locations} ${ttl}` | ||
| ); | ||
| assert.match(output, new RegExp(`Created secret: ${secret.name}`)); |
There was a problem hiding this comment.
The assertion in this test is incorrect. It compares the output with secret.name, which refers to a secret created in the before hook with a different ID. The test creates a new secret with the ID ${secretId}-ummr, so the assertion should check for the name of this newly created secret.
| assert.match(output, new RegExp(`Created secret: ${secret.name}`)); | |
| assert.match( | |
| output, | |
| new RegExp(`Created secret: ${parent}/secrets/${secretId}-ummr`) | |
| ); |
There was a problem hiding this comment.
we are already passing secretId
| } | ||
|
|
||
| const args = process.argv.slice(2); | ||
| const locations = args[2] ? args[2].split(',') : []; |
There was a problem hiding this comment.
The command-line argument parsing is brittle. If an empty string is passed for locations (e.g., to specify a ttl without locations), args[2].split(',') results in [''], which will likely cause an API error. A more robust way to handle this is to filter out empty strings from the locations array.
const locations = args[2] ? args[2].split(',').filter(l => l) : [];There was a problem hiding this comment.
It is passed by user and empty string should not be given into replication locations
| const secretConfig = { | ||
| replication: { | ||
| userManaged: { | ||
| replicas: locations.map(location => ({location})), | ||
| }, | ||
| }, | ||
| ttl: { | ||
| seconds: ttl, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
When ttl is null, the secretConfig object will have ttl: { seconds: null }. This might cause an API error if the backend expects the ttl field to be absent or to contain a valid duration. It's safer to conditionally add the ttl property to the secretConfig object only when a ttl value is provided.
const secretConfig = {
replication: {
userManaged: {
replicas: locations.map(location => ({location})),
},
},
};
if (ttl) {
secretConfig.ttl = {
seconds: ttl,
};
}| async function listSecretsWithFilter() { | ||
| const [secrets] = await client.listSecrets({ | ||
| parent: parent, | ||
| filter: filterStr, | ||
| }); | ||
|
|
||
| // Print each secret | ||
| for (const secret of secrets) { | ||
| console.log(`Found secret: ${secret.name}`); | ||
| } | ||
| } | ||
|
|
||
| listSecretsWithFilter(); |
There was a problem hiding this comment.
The main function is already async, so the nested async function listSecretsWithFilter which is defined and immediately called is redundant. You can simplify the code by moving the logic from listSecretsWithFilter directly into main. This will make the code more concise and easier to read.
const [secrets] = await client.listSecrets({
parent: parent,
filter: filterStr,
});
// Print each secret
for (const secret of secrets) {
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.
| async function detachTag() { | ||
| // Find the binding name for the given tag value | ||
| let bindingName = null; | ||
| const iterable = rmClient.listTagBindingsAsync( | ||
| { | ||
| parent: parent, | ||
| pageSize: 50, | ||
| }, | ||
| {autoPaginate: false} | ||
| ); | ||
|
|
||
| for await (const binding of iterable) { | ||
| if (binding.tagValue === tagValue) { | ||
| bindingName = binding.name; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (bindingName === null) { | ||
| console.log(`Tag binding for value ${tagValue} not found on ${name}.`); | ||
| return; | ||
| } | ||
|
|
||
| // Delete the tag binding | ||
| const [operation] = await rmClient.deleteTagBinding({ | ||
| name: bindingName, | ||
| }); | ||
|
|
||
| // Wait for the operation to complete | ||
| await operation.promise(); | ||
| console.log(`Detached tag value ${tagValue} from ${name}`); | ||
| } | ||
|
|
||
| detachTag(); |
There was a problem hiding this comment.
The main function is already async, so the nested async function detachTag which is defined and immediately called is redundant. You can simplify the code by moving the logic from detachTag directly into main. This will make the code more concise and easier to read.
// Find the binding name for the given tag value
let bindingName = null;
const iterable = rmClient.listTagBindingsAsync(
{
parent: parent,
pageSize: 50,
},
{autoPaginate: false}
);
for await (const binding of iterable) {
if (binding.tagValue === tagValue) {
bindingName = binding.name;
break;
}
}
if (bindingName === null) {
console.log(`Tag binding for value ${tagValue} not found on ${name}.`);
return;
}
// Delete the tag binding
const [operation] = await rmClient.deleteTagBinding({
name: bindingName,
});
// Wait for the operation to complete
await operation.promise();
console.log(`Detached tag value ${tagValue} from ${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 detachRegionalTag() { | ||
| // Find the binding with the specified tag value | ||
| let bindingName = null; | ||
| const iterable = tagBindingsClient.listTagBindingsAsync( | ||
| { | ||
| parent: parent, | ||
| pageSize: 50, | ||
| }, | ||
| {autoPaginate: false} | ||
| ); | ||
|
|
||
| for await (const binding of iterable) { | ||
| if (binding.tagValue === tagValue) { | ||
| bindingName = binding.name; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (bindingName === null) { | ||
| console.log( | ||
| `Tag binding for value ${tagValue} not found on ${secretName}.` | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| // Delete the tag binding | ||
| const [operation] = await tagBindingsClient.deleteTagBinding({ | ||
| name: bindingName, | ||
| }); | ||
|
|
||
| // Wait for the operation to complete | ||
| await operation.promise(); | ||
|
|
||
| console.log(`Detached tag value ${tagValue} from ${secretName}`); | ||
| } | ||
|
|
||
| return detachRegionalTag(); |
There was a problem hiding this comment.
The main function is already async, so the nested async function detachRegionalTag which is defined and immediately called is redundant. You can simplify the code by moving the logic from detachRegionalTag directly into main. This will make the code more concise and easier to read.
// Find the binding with the specified tag value
let bindingName = null;
const iterable = tagBindingsClient.listTagBindingsAsync(
{
parent: parent,
pageSize: 50,
},
{autoPaginate: false}
);
for await (const binding of iterable) {
if (binding.tagValue === tagValue) {
bindingName = binding.name;
break;
}
}
if (bindingName === null) {
console.log(
`Tag binding for value ${tagValue} not found on ${secretName}.`
);
return;
}
// Delete the tag binding
const [operation] = await tagBindingsClient.deleteTagBinding({
name: bindingName,
});
// Wait for the operation to complete
await operation.promise();
console.log(`Detached tag value ${tagValue} from ${secretName}`);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 already async, so the nested async function listRegionalSecretVersionsWithFilter which is defined and immediately called is redundant. You can simplify the code by moving the logic from listRegionalSecretVersionsWithFilter directly into main. This will make the code more concise and easier to read.
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 already async, so the nested async function listRegionalSecretsWithFilter which is defined and immediately called is redundant. You can simplify the code by moving the logic from listRegionalSecretsWithFilter directly into main. This will make the code more concise and easier to read.
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.
| async function listRegionalSecretTagBindings() { | ||
| console.log(`Tag bindings for ${name}:`); | ||
| let foundBindings = false; | ||
|
|
||
| // List the tag bindings | ||
| const iterable = tagBindingsClient.listTagBindingsAsync( | ||
| { | ||
| parent: parent, | ||
| pageSize: 10, | ||
| }, | ||
| {autoPaginate: false} | ||
| ); | ||
|
|
||
| // Iterate through the results | ||
| for await (const binding of iterable) { | ||
| console.log(`- Tag Value: ${binding.tagValue}`); | ||
| foundBindings = true; | ||
| } | ||
|
|
||
| if (!foundBindings) { | ||
| console.log(`No tag bindings found for ${name}.`); | ||
| } | ||
| } | ||
|
|
||
| listRegionalSecretTagBindings(); |
There was a problem hiding this comment.
The main function is already async, so the nested async function listRegionalSecretTagBindings which is defined and immediately called is redundant. You can simplify the code by moving the logic from listRegionalSecretTagBindings directly into main. This will make the code more concise and easier to read.
console.log(`Tag bindings for ${name}:`);
let foundBindings = false;
// List the tag bindings
const iterable = tagBindingsClient.listTagBindingsAsync(
{
parent: parent,
pageSize: 10,
},
{autoPaginate: false}
);
// Iterate through the results
for await (const binding of iterable) {
console.log(`- Tag Value: ${binding.tagValue}`);
foundBindings = true;
}
if (!foundBindings) {
console.log(`No tag bindings found for ${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 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 already async, so the nested async function createUmmrSecret which is defined and immediately called is redundant. You can simplify the code by moving the logic from createUmmrSecret directly into main. This will make the code more concise and easier to read.
// 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.
1 participant
Adding cmek 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.