feat(parametermanager): Added samples for kms_key field #5240
Conversation
product-auto-label
bot
added
the
samples
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @vatsal-vora-crestdata, 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 introduces samples for the kms_key field within the Parameter Manager SDK. It includes functionalities for creating, updating, and removing the kms_key associated with a parameter. The changes involve adding new Go files for each of these operations, updating the go.mod and go.sum files to include necessary dependencies, and adding corresponding tests to ensure the new functionalities work as expected.
Highlights
- New Samples: Added three new samples:
create_param_with_kms_key,update_param_kms_key, andremove_param_kms_keyto demonstrate the usage of thekms_keyfield. - Dependency Updates: The
go.modandgo.sumfiles have been updated to include the necessary dependencies for the Parameter Manager and KMS APIs. - Testing: Added new tests to verify the functionality of the new samples, including creating KMS keys and key rings for testing purposes.
Changelog
Click here to see the changelog
- go.work
- Added the
parametermanagerdirectory to thego.workfile to include it in the workspace.
- Added the
- parametermanager/create_param_with_kms_key.go
- Created a new function
createParamWithKmsKeythat creates a parameter with a KMS key. - The function takes projectID, parameterID, and kmsKey as input.
- It uses the Parameter Manager SDK to create the parameter with the specified KMS key.
- Created a new function
- parametermanager/go.mod
- Added dependencies for
cloud.google.com/go/kmsandcloud.google.com/go/parametermanager.
- Added dependencies for
- parametermanager/go.sum
- Added checksums for the new dependencies.
- parametermanager/parametermanager_test.go
- Added new tests for creating, updating, and removing the
kms_keyfield of a parameter. - Includes helper functions for creating KMS key rings and keys for testing.
- Added cleanup functions to delete the created resources after testing.
- Added new tests for creating, updating, and removing the
- parametermanager/remove_param_kms_key.go
- Created a new function
removeParamKmsKeythat removes the KMS key from a parameter. - The function takes projectID and parameterID as input.
- It uses the Parameter Manager SDK to update the parameter and remove the KMS key.
- Created a new function
- parametermanager/update_param_kms_key.go
- Created a new function
updateParamKmsKeythat updates the KMS key of a parameter. - The function takes projectID, parameterID, and kmsKey as input.
- It uses the Parameter Manager SDK to update the parameter with the specified KMS key.
- Created a new function
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 KMS key,
A shield for secrets kept,
Parameter's guard.
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 samples for the kms_key field in the Parameter Manager SDK. The changes include creating, updating, and removing parameters with KMS keys, along with corresponding tests. Overall, the code is well-structured and addresses the feature request. However, there are a few areas that could be improved for clarity and efficiency.
Summary of Findings
- Error Handling in Tests: The test functions
testCleanupParameterandtestCleanupKeyVersionsuset.Fatalfwhen a deletion fails, even if the resource is not found. This can cause tests to fail unnecessarily. It's better to log the error and continue if the resource is not found. - KMS Key Version Cleanup: The
testCleanupKeyVersionsfunction attempts to destroy a crypto key version, but the sample code doesn't create any key versions. This cleanup function might not be necessary or might need adjustment based on how key versions are managed in the samples. - Redundant Parameter Format Setting: In
removeParamKmsKey, theParameterstruct in theUpdateParameterRequestsets theFormatfield toUNFORMATTED, which might be redundant since the goal is to remove the KMS key, not change the format.
Merge Readiness
The pull request is almost ready for merging. Addressing the error handling in the cleanup functions and reviewing the necessity of the testCleanupKeyVersions function would improve the reliability and clarity of the tests. The redundant parameter format setting in removeParamKmsKey should also be addressed. I am unable to approve the pull request, and users should have others review and approve this code before merging. Once these points are addressed, the pull request should be in good shape to be merged.
| }); err != nil { | ||
| if terr, ok := grpcstatus.FromError(err); !ok || terr.Code() != grpccodes.NotFound { | ||
| t.Fatalf("testCleanupParameter: failed to delete parameter: %v", err) | ||
| } |
There was a problem hiding this comment.
Instead of t.Fatalf, consider using t.Logf or t.Errorf and returning. This allows the test to continue and potentially clean up other resources, even if one deletion fails because the resource was already gone. This makes the tests more robust.
if terr, ok := grpcstatus.FromError(err); ok && terr.Code() == grpccodes.NotFound {
t.Logf("testCleanupParameter: parameter %s not found, skipping deletion", name)
return
}
t.Fatalf("testCleanupParameter: failed to delete parameter: %v", err)| }); err != nil { | ||
| if terr, ok := grpcstatus.FromError(err); !ok || terr.Code() != grpccodes.NotFound { | ||
| t.Fatalf("testCleanupKeyVersion: failed to delete key version: %v", err) | ||
| } |
There was a problem hiding this comment.
Similar to testCleanupParameter, use t.Logf or t.Errorf instead of t.Fatalf to allow the test to continue even if the key version is not found. Also, double check if this cleanup is actually needed, since the sample code doesn't create any key versions.
if terr, ok := grpcstatus.FromError(err); ok && terr.Code() == grpccodes.NotFound {
t.Logf("testCleanupKeyVersion: key version %s not found, skipping deletion", name)
return
}
t.Fatalf("testCleanupKeyVersion: failed to delete key version: %v", err)
|
Raised a duplicate of this PR #5261 because Vatsal's CLA has expired, which is blocking the merge. Once the new PR is merged, will close the original one. |
Description
Created samples for kms_key field using Parameter Manager SDK
Sample List:
Added required Tests for the same.
Checklist
go test -v ./..(see Testing)gofmt(see Formatting)go vet(see Formatting)GOLANG_REGIONAL_SAMPLES_LOCATIONvariable to be set