-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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(secretmanager): add optional ttl to create secret sample #9889
feat(secretmanager): add optional ttl to create secret sample #9889
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alarconesparza and reviewers,
I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help everyone get up to speed:
This pull request enhances the secretmanager
sample by adding an optional Time-To-Live (TTL) parameter to the createSecret
function. The intent is to provide users with more control over the lifecycle of their secrets, allowing them to specify an automatic deletion time.
Here's a breakdown of the changes:
- File Changed:
secretmanager/src/main/java/secretmanager/CreateSecret.java
- Lines Changed: 21-25, 41-60
- Summary of Changes: The
CreateSecret
class now importscom.google.protobuf.Duration
. A newDuration
object,ttl
, is created and set to 900 seconds (15 minutes). Thisttl
object is then added to theSecret
object being created, giving the secret a 15-minute TTL. This means that secret versions will be automatically deleted after 15 minutes.
The changes are well-structured and clearly documented. The addition of the TTL option enhances the functionality of the sample and provides users with greater flexibility in managing their secrets.
And now, a little haiku to lighten the mood:
Secrets come and go,
TTL sets their fleeting time,
Code's clean, all is well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request adds an optional TTL to the CreateSecret
sample. This is a useful addition, making the sample more comprehensive. The implementation looks correct, but there are a few minor suggestions for improvement based on the provided Sample Format Guide and general Java best practices.
It's great that you've included the checklist in the description. Please ensure all items are checked before merging.
// Optionally add a TTL that specifies the secret's time-to-live in seconds | ||
// (e.g., 900 for 15 minutes). If specified, the secret versions will be | ||
// automatically deleted upon reaching the end of the TTL period. | ||
Duration ttl = Duration.newBuilder().setSeconds(900).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making the TTL configurable via a command-line argument or an environment variable. This would make the sample more flexible and allow users to experiment with different TTL values. This aligns with the Sample Format Guide's recommendation for user-provided parameters. For example, you could use System.getenv("SECRET_TTL")
to read the TTL from an environment variable.
// Optionally add a TTL that specifies the secret's time-to-live in seconds | ||
// (e.g., 900 for 15 minutes). If specified, the secret versions will be | ||
// automatically deleted upon reaching the end of the TTL period. | ||
Duration ttl = Duration.newBuilder().setSeconds(900).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good practice to validate user inputs, especially when they can affect the behavior of the system. If you make the TTL configurable, add validation to ensure the provided value is within an acceptable range and is a valid integer. For example, you might want to enforce a minimum and maximum TTL and handle potential NumberFormatException
if the input is not a valid integer. This also aligns with the Sample Format Guide, which emphasizes clear error handling.
Co-authored-by: code-review-assist[bot] <182814678+code-review-assist[bot]@users.noreply.github.com>
@alarconesparza apologies, but the committed change from the code-review-assist for the comment messed up the indentation, could you fix that in a an additional commit? Thank you! |
Hi @iennae, thanks for pointing out the indentation issue. I apologize for the formatting problem introduced by my previous suggestion. Unfortunately, I can't directly fix the code in the repository. Could you please re-commit the changes with the corrected indentation? Let me know if you need any further assistance. |
Sure, let me update the comment message on my side and commit it with the correct identation |
Hi @alarconesparza, Great that you're addressing the indentation issue! To help ensure clarity for future contributors and reviewers, consider adding a comment in the code itself explaining the purpose of the |
* chore(job): migrate regions by associating them with an official product with a job_ prefix (#9883) * chore(endpoints): delete region 'swagger' in endpoints/multiple-versions (#9857) * chore(endpoints): delete region swagger to openapi-v1.yaml * chore(endpoints): delete region swagger to openapi-v2.yaml * chore(job): delete sample jobs_java_dependencies_beta (#9810) * chore(job): delete sample jobs_java_dependencies_beta * chore(job): delete region_tab 'jobs_java_dependencies_beta' and update 'google-api-services-jobs' version * feat(compute): add compute disk regional replicated sample (#9697) * Implemented compute_disk_regional_replicated sample, created test * Fixed zone * Fixed test * Fixed test * Fixed disk size * Fixed code as requested in the comment * feat(compute): add compute disk start/stop replication samples (#9650) * Implemented compute_disk_start_replication and compute_disk_stop_replication samples, created tests * Fixed test * Deleted not related classes * Fixed lint issue * Increased timeout * Split samples for zonal location * Fixed code * Fixed code * Increased timeout * Increased timeout * feat(tpu): add tpu vm create spot sample. (#9610) * Changed package, added information to CODEOWNERS * Added information to CODEOWNERS * Added timeout * Fixed parameters for test * Fixed DeleteTpuVm and naming * Added comment, created Util class * Fixed naming * Fixed whitespace * Split PR into smaller, deleted redundant code * Implemented tpu_vm_create_spot sample, created test * changed zone * Changed zone * Fixed empty lines and tests, deleted cleanup method * Changed zone * Deleted redundant test class * Increased timeout * Fixed test * feat(tpu): add tpu vm create startup script sample. (#9612) * Changed package, added information to CODEOWNERS * Added information to CODEOWNERS * Added timeout * Fixed parameters for test * Fixed DeleteTpuVm and naming * Added comment, created Util class * Fixed naming * Fixed whitespace * Split PR into smaller, deleted redundant code * Implemented tpu_vm_create_startup_script sample, created test * Fixed tests and empty lines * Changed zone * Deleted redundant test classes * Increased timeout * Fixed code * feat(tpu): add tpu queued resources create/get/delete samples (#9613) * Changed package, added information to CODEOWNERS * Added information to CODEOWNERS * Added timeout * Fixed parameters for test * Fixed DeleteTpuVm and naming * Added comment, created Util class * Fixed naming * Fixed whitespace * Split PR into smaller, deleted redundant code * Implemented tpu_queued_resources_create, tpu_queued_resources_get, tpu_queued_resources_delete_force and tpu_queued_resources_delete samples, created tests * Fixed test * Fixed tests * Fixed error massage * Fixed typo * Fixed zone * Fixed test * Fixed code * Deleted commented imports * Fixed code as requested in comments * feat(tpu): add tpu queued resources create spot (#9615) Add a code sample for tpu_queued_resources_create_spot * chore: add translate dev team for translate samples (#9888) b/385243174 * feat(securitycenter): Add Resource SCC Management API Org ETD Custom Module code samples (Create, Delete, List, Get) (#9743) * sample codes for event threat detection custom modules * addressed comments * addressed comments * addressed comments * addressed comments * fix(compute): fixed compute_reservation_create_shared sample and test to use mocked client (#9840) * Fixed sample and test to use mocked client * Fixed code as requested in the comments * feat(compute): add compute instance create replicated boot disk sample (#9735) * Implemented compute_instance_create_replicated_boot_disk sample, created test * Fixed test * Fixed code as requested in the comments * Fixed Util class * Fixed code * feat(compute): add compute consistency group stop replication (#9694) * Implemented compute_consistency_group_create and compute_consistency_group_delete samples, created test * Implemented compute_consistency_group_stop_replication sample * Implemented compute_consistency_group_stop_replication sample * Created test and added needed classes for testing * Fixed test * Moved clean up methods * Added clean up methods for reservations * Fixed clean up method * Fixed clean up method * Added timeout * Reverted not related changes * Reverted not related changes * Reverted not related changes * Reverted not related changes * Fixed code * Split samples for zonal location * Added comments for methods * Fixed comments * feat(secretmanager): add optional ttl to create secret sample (#9889) * feat(secretmanager): add optional ttl to create secret sample * nit: Update secretmanager/src/main/java/secretmanager/CreateSecret.java Co-authored-by: code-review-assist[bot] <182814678+code-review-assist[bot]@users.noreply.github.com> * fix(secretmanager): fix comment indentation to resolve linting issues --------- Co-authored-by: Jennifer Davis <[email protected]> Co-authored-by: code-review-assist[bot] <182814678+code-review-assist[bot]@users.noreply.github.com> * feat(tpu): add tpu queued resources list sample (#9614) * Changed package, added information to CODEOWNERS * Added information to CODEOWNERS * Added timeout * Fixed parameters for test * Fixed DeleteTpuVm and naming * Added comment, created Util class * Fixed naming * Fixed whitespace * Split PR into smaller, deleted redundant code * Implemented tpu_queued_resources_create, tpu_queued_resources_get, tpu_queued_resources_delete_force and tpu_queued_resources_delete samples, created tests * Implemented tpu_queued_resources_list sample, created test * Fixed test * Fixed tests, deleted cleanup method * Fixed test * Fixed imports * feat(compute): add compute disk create secondary regional sample (#9641) * Implemented compute_disk_create_secondary_regional. created test * Fixed test * Fixed test * Fixed test * Fixed zone * Fixed naming * Fixed spaces * Fixed code * Fixed indentations * Fixed variable * Fixed code * Added cleanup methods * Fixed lint issue * Fixed lint issue * Fixed test * Fixed code * Fixed code * Fixed code * Deleted duplicated assertion * feat(compute): add compute disk create secondary sample. (#9643) * Implemented compute_disk_create_secondary sample, created test * Fixed code * Fixed variable * Fixed code * Merged changes from main * Fixed lint issue * fix(storage): migrate old region all to storagetransfer_transfer_all step 1 (#9917) * fix(job): remove old region create_job (#9914) * feat(compute): attach/ remove snapshot schedule to disk (#9791) * Implemented compute_snapshot_schedule_attach sample, created test * Implemented compute_snapshot_schedule_remove sample, created test * Fixed code * Fixed code as requested in the comments * feat(compute): add compute consistency group clone sample (#9885) * Implemented compute_consistency_group_clone and compute_consistency_group_clone_regional_disk samples, created tests * Fixed naming * feat(compute): add compute instance attach regional disk force sample (#9730) * Implemented compute_instance_attach_regional_disk_force sample, created test * Added clean up method * Fixed comments and parameters * Test order deleted * Fixed code * Fixed code * Fixed code * Increased timeout * Increased timeout * Increased timeout * Fixed code * Fixed code * Fixed code * Fixed naming * feat(compute): add compute disk create secondary custom sample (#9644) * Implemented compute_disk_create_secondary_custom sample, created test * Fixed code * Fixed variable * Fixed code * Fixed whitespace * Fixed whitespace * feat(compute): add compute snapshot schedule create/get/edit/list/delete samples (#9742) * Implemented compute_snapshot_schedule_delete and compute_snapshot_schedule_create samples, created test * Fixed test * Added compute_snapshot_schedule_get sample, created test * Fixed naming * Implemented compute_snapshot_schedule_edit, created test * Fixed naming * Implemented compute_snapshot_schedule_list sample, created test * Cleaned resources * Cleaned resources * Cleaned resources * Cleaned resources * Fixed test * Added comment * Fixed tests * Fixed code * Fixed code as requested in the comments * feat(compute): add compute disk create with snapshot schedule (#9788) * Implemented compute_disk_create_with_snapshot_schedule sample, created test * Fixed code * Fixed code * Fixed test * Fixed code * Fixed code as requested in the comments * Fixed lint issue * Fixed lint issue * Deleted redundant code * feat(tpu): add tpu queued resources time bound sample. (#9617) * Changed package, added information to CODEOWNERS * Added information to CODEOWNERS * Added timeout * Fixed parameters for test * Fixed DeleteTpuVm and naming * Added comment, created Util class * Fixed naming * Fixed whitespace * Split PR into smaller, deleted redundant code * Implemented tpu_queued_resources_create, tpu_queued_resources_get, tpu_queued_resources_delete_force and tpu_queued_resources_delete samples, created tests * Implemented tpu_queued_resources_time_bound sample, created test * Changed zone for tpu * Cleanup resources * Fixed tests * Fixed test * Fixed code as requested in the comments * Fixed code as requested in the comments * fix(job): delete old region tag update_job_with_field_mask (#9940) * feat(job): migrate region tags to include product prefix (#9966) * fix(endpoints): migrate all regions (#9943) * fix: disable flakybot reporting (#9968) * chore(job): remove unused region tags (#9969) * feat(securitycenter): Add Resource SCC Management API Org ETD Custom Module code samples (Update, Get Eff, List Eff, List Desc, Validate) (#9912) * sample codes for event threat detection custom modules * fixed lint * addressed comments * lint fix * addressed comments --------- Co-authored-by: OremGLG <[email protected]> Co-authored-by: eapl.me <[email protected]> Co-authored-by: Тетяна Ягодська <[email protected]> Co-authored-by: Jennifer Davis <[email protected]> Co-authored-by: lovenishs04 <[email protected]> Co-authored-by: alarconesparza <[email protected]> Co-authored-by: Jennifer Davis <[email protected]> Co-authored-by: code-review-assist[bot] <182814678+code-review-assist[bot]@users.noreply.github.com> Co-authored-by: Brian Dorsey <[email protected]>
Description
Fixes
Internal b/384051649
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
pom.xml
parent set to latestshared-configuration
mvn clean verify
requiredmvn -P lint checkstyle:check
requiredmvn -P lint clean compile pmd:cpd-check spotbugs:check
advisory only