Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(secret-manager): add optional ttl field to createSecret function #3937

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hivanalejandro
Copy link

Description

  • Introduced the 'ttl' parameter in the Node.js sample for creating secrets.
  • Ensured backward compatibility by handling cases where 'ttl' is undefined.

Fixes b-384052815

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

  • I have followed guidelines from CONTRIBUTING.MD and Samples Style Guide
  • Tests pass: npm test (see Testing)
  • Lint pass: npm run lint (see Style)
  • These samples need a new API enabled in testing projects to pass (let us know which ones)
  • These samples need a new/updated env vars in testing projects set to pass (let us know which ones)
  • This pull request is from a branch created directly off of GoogleCloudPlatform/nodejs-docs-samples. Not a fork.
  • This sample adds a new sample directory, and I updated the CODEOWNERS file with the codeowners for this sample
  • This sample adds a new sample directory, and I created GitHub Actions workflow for this sample
  • This sample adds a new Product API, and I updated the Blunderbuss issue/PR auto-assigner with the codeowners for this sample
  • Please merge this PR for me once it is approved

- Introduced the 'ttl' parameter in the Node.js sample for creating secrets.
- Ensured backward compatibility by handling cases where 'ttl' is undefined.
@hivanalejandro hivanalejandro requested review from a team as code owners December 19, 2024 21:21
Copy link
Contributor

Gemini encountered an error creating the summary. You can try again by commenting @code-review-assist summarize.

@product-auto-label product-auto-label bot added api: secretmanager Issues related to the Secret Manager API. samples Issues that are directly related to samples. labels Dec 19, 2024
async function main(
parent = 'projects/my-project',
secretId = 'my-secret',
ttl = undefined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case, could you go ahead and set it to 900s instead of undefined?

@iennae
Copy link
Contributor

iennae commented Dec 19, 2024

@code-review-assist review

Copy link
Contributor

This pull request adds an optional ttl field to the createSecret function in the Node.js Secret Manager sample. The changes are well-structured and include handling for undefined ttl values, ensuring backward compatibility. The code clearly shows how to specify the TTL in seconds. I recommend adding a comment above the ttl parameter in the main function explaining the format of the TTL string (e.g., '900s' for 15 minutes, '1h' for 1 hour, etc.). This will improve clarity for users. Also, consider adding a test case to verify the functionality of the ttl parameter. Overall, this is a good change and addresses the issue effectively.

@iennae
Copy link
Contributor

iennae commented Dec 20, 2024

@code-review-assist walkthrough

Copy link
Contributor

Gemini encountered an error creating the walkthrough. You can try again by commenting @code-review-assist walkthrough.

@eddie-santos
Copy link
Member

@code-review-assist summarize

Copy link
Contributor

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi team,

I'm currently reviewing this pull request and will have a full review ready in a few minutes. In the meantime, here's a quick summary to help you and other reviewers get up to speed:

This pull request, authored by @hivanalejandro, enhances the secret-manager/createSecret.js sample by adding an optional ttl (time-to-live) parameter to the createSecret function. This allows users to specify a lifespan for their secrets.

Here's a breakdown of the changes:

  • Intent: Improve flexibility and security by allowing users to set an expiration time for secrets created using the Node.js sample.
  • Changes:
    • Added a new ttl parameter to the main function in secret-manager/createSecret.js.
    • Modified the createSecret function to accept and use the ttl parameter when creating a secret. It also handles cases where ttl is undefined (for backward compatibility).
    • Updated comments to reflect the new ttl parameter and its usage.

The changes are well-structured and clearly documented. The addition of the optional ttl parameter is a valuable enhancement that improves the usability and security of the createSecret function.


And now, a little haiku to lighten the mood:

Secrets short or long,
Time to live, a choice is made,
Code flows, secure.

Thanks,
Gemini

@eddie-santos
Copy link
Member

@code-review-assist walkthrough

Copy link
Contributor

Gemini encountered an error creating the walkthrough. You can try again by commenting @code-review-assist walkthrough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: secretmanager Issues related to the Secret Manager API. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants