feat(parametermanager): Added samples for delete, enable or disable parameter or parameter version in specified region #10049
Conversation
|
Here is the summary of changes. You are about to add 4 region tags.
This comment is generated by snippet-bot.
|
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 interacting with the Parameter Manager SDK, specifically focusing on regional parameters. I've added samples for deleting a parameter, enabling/disabling a parameter version, and deleting a parameter version. Each sample includes the necessary code to connect to the regional Parameter Manager server and perform the specified action. Additionally, I've included corresponding tests to ensure the samples function as expected.
Highlights
- New Samples: Added four new regional samples:
DeleteRegionalParam,DisableRegionalParamVersion,EnableRegionalParamVersion, andDeleteRegionalParamVersion. - Functionality: The samples demonstrate how to delete parameters, enable/disable parameter versions, and delete parameter versions within a specified region using the Parameter Manager SDK.
- Testing: Included integration tests (
SnippetsIT.java) to verify the functionality of the new samples.
Changelog
Click here to see the changelog
- parametermanager/pom.xml
- Added dependencies for google-cloud-parametermanager, protobuf-java-util, lombok, junit, truth, google-cloud-secretmanager, and google-iam-policy.
- Configured the maven-compiler-plugin to use source and target 11, and added lombok to annotationProcessorPaths.
- parametermanager/src/main/java/parametermanager/regionalsamples/DeleteRegionalParam.java
- Created a new sample to delete a regional parameter.
- The sample connects to the regional Parameter Manager server and deletes the specified parameter.
- parametermanager/src/main/java/parametermanager/regionalsamples/DeleteRegionalParamVersion.java
- Created a new sample to delete a regional parameter version.
- The sample connects to the regional Parameter Manager server and deletes the specified parameter version.
- parametermanager/src/main/java/parametermanager/regionalsamples/DisableRegionalParamVersion.java
- Created a new sample to disable a regional parameter version.
- The sample connects to the regional Parameter Manager server and disables the specified parameter version by updating the
disabledfield.
- parametermanager/src/main/java/parametermanager/regionalsamples/EnableRegionalParamVersion.java
- Created a new sample to enable a regional parameter version.
- The sample connects to the regional Parameter Manager server and enables the specified parameter version by updating the
disabledfield.
- parametermanager/src/test/java/parametermanager/regionalsamples/SnippetsIT.java
- Added integration tests for the new regional samples.
- The tests verify the functionality of deleting parameters, enabling/disabling parameter versions, and deleting parameter versions.
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.
In clouds of code, parameters reside,
Managed with care, in regions applied.
Versions enabled, or softly disabled,
A digital dance, precisely enabled.
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 deleting, enabling, and disabling parameters or parameter versions using the Parameter Manager SDK. The changes include new classes for each operation and corresponding tests. Overall, the code seems well-structured and follows the expected pattern for samples in this repository.
Summary of Findings
- Missing Exception Handling: The samples lack specific exception handling for API calls. While a general IOException is caught, more specific exceptions related to the Parameter Manager API should be handled to provide better error reporting and resilience.
- Hardcoded API Endpoint: The API endpoint is constructed using string formatting. While functional, it would be more robust to use a dedicated method or configuration to ensure the endpoint is correctly formed, especially when dealing with different regions or environments.
- Inconsistent Test Naming: The test methods in
SnippetsIT.javacould benefit from more descriptive names that clearly indicate what is being tested. This would improve the readability and maintainability of the test suite.
Merge Readiness
The pull request is almost ready for merging. However, addressing the missing exception handling and hardcoded API endpoint issues would significantly improve the robustness and maintainability of the samples. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. At a minimum, the high severity issues should be addressed before merging.
parametermanager/src/main/java/parametermanager/regionalsamples/DeleteRegionalParam.java
Show resolved
Hide resolved
parametermanager/src/main/java/parametermanager/regionalsamples/DeleteRegionalParamVersion.java
Show resolved
Hide resolved
...metermanager/src/main/java/parametermanager/regionalsamples/DisableRegionalParamVersion.java
Show resolved
Hide resolved
parametermanager/src/main/java/parametermanager/regionalsamples/EnableRegionalParamVersion.java
Show resolved
Hide resolved
parametermanager/src/main/java/parametermanager/regionalsamples/DeleteRegionalParam.java
Show resolved
Hide resolved
parametermanager/src/main/java/parametermanager/regionalsamples/EnableRegionalParamVersion.java
Show resolved
Hide resolved
| @Test | ||
| public void testDisableRegionalParameterVersion() throws IOException { | ||
| ParameterVersionName parameterVersionName = TEST_PARAMETER_VERSION_NAME_TO_GET_1; | ||
| DisableRegionalParamVersion.disableRegionalParamVersion( | ||
| parameterVersionName.getProject(), | ||
| parameterVersionName.getLocation(), | ||
| parameterVersionName.getParameter(), | ||
| parameterVersionName.getParameterVersion()); | ||
|
|
||
| assertThat(stdOut.toString()).contains("Disabled regional parameter version"); | ||
| } |
There was a problem hiding this comment.
The test method name testDisableRegionalParameterVersion is not very descriptive. Consider renaming it to something more specific, such as testDisableRegionalParameterVersion_success or testDisableRegionalParameterVersion_parameterVersionIsDisabled to clearly indicate what the test is verifying.
| @Test | |
| public void testDisableRegionalParameterVersion() throws IOException { | |
| ParameterVersionName parameterVersionName = TEST_PARAMETER_VERSION_NAME_TO_GET_1; | |
| DisableRegionalParamVersion.disableRegionalParamVersion( | |
| parameterVersionName.getProject(), | |
| parameterVersionName.getLocation(), | |
| parameterVersionName.getParameter(), | |
| parameterVersionName.getParameterVersion()); | |
| assertThat(stdOut.toString()).contains("Disabled regional parameter version"); | |
| } | |
| @Test | |
| public void testDisableRegionalParameterVersion_success() throws IOException { |
| @Test | ||
| public void testEnableRegionalParameterVersion() throws IOException { | ||
| ParameterVersionName parameterVersionName = TEST_PARAMETER_VERSION_NAME_TO_GET_1; | ||
| EnableRegionalParamVersion.enableRegionalParamVersion( | ||
| parameterVersionName.getProject(), | ||
| parameterVersionName.getLocation(), | ||
| parameterVersionName.getParameter(), | ||
| parameterVersionName.getParameterVersion()); | ||
|
|
||
| assertThat(stdOut.toString()).contains("Enabled regional parameter version"); | ||
| } |
There was a problem hiding this comment.
The test method name testEnableRegionalParameterVersion is not very descriptive. Consider renaming it to something more specific, such as testEnableRegionalParameterVersion_success or testEnableRegionalParameterVersion_parameterVersionIsEnabled to clearly indicate what the test is verifying.
| @Test | |
| public void testEnableRegionalParameterVersion() throws IOException { | |
| ParameterVersionName parameterVersionName = TEST_PARAMETER_VERSION_NAME_TO_GET_1; | |
| EnableRegionalParamVersion.enableRegionalParamVersion( | |
| parameterVersionName.getProject(), | |
| parameterVersionName.getLocation(), | |
| parameterVersionName.getParameter(), | |
| parameterVersionName.getParameterVersion()); | |
| assertThat(stdOut.toString()).contains("Enabled regional parameter version"); | |
| } | |
| @Test | |
| public void testEnableRegionalParameterVersion_success() throws IOException { |
| @Test | ||
| public void testDeleteRegionalParameterVersion() throws IOException { | ||
| ParameterVersionName parameterVersionName = TEST_PARAMETER_VERSION_NAME_TO_DELETE; | ||
| DeleteRegionalParamVersion.deleteRegionalParamVersion( | ||
| parameterVersionName.getProject(), | ||
| parameterVersionName.getLocation(), | ||
| parameterVersionName.getParameter(), | ||
| parameterVersionName.getParameterVersion()); | ||
|
|
||
| assertThat(stdOut.toString()).contains("Deleted regional parameter version:"); | ||
| } |
There was a problem hiding this comment.
The test method name testDeleteRegionalParameterVersion is not very descriptive. Consider renaming it to something more specific, such as testDeleteRegionalParameterVersion_success or testDeleteRegionalParameterVersion_parameterVersionIsDeleted to clearly indicate what the test is verifying.
| @Test | |
| public void testDeleteRegionalParameterVersion() throws IOException { | |
| ParameterVersionName parameterVersionName = TEST_PARAMETER_VERSION_NAME_TO_DELETE; | |
| DeleteRegionalParamVersion.deleteRegionalParamVersion( | |
| parameterVersionName.getProject(), | |
| parameterVersionName.getLocation(), | |
| parameterVersionName.getParameter(), | |
| parameterVersionName.getParameterVersion()); | |
| assertThat(stdOut.toString()).contains("Deleted regional parameter version:"); | |
| } | |
| @Test | |
| public void testDeleteRegionalParameterVersion_success() throws IOException { |
| @Test | ||
| public void testDeleteRegionalParameter() throws IOException { | ||
| ParameterName parameterName = TEST_PARAMETER_NAME_TO_DELETE; | ||
| DeleteRegionalParam.deleteRegionalParam( | ||
| parameterName.getProject(), parameterName.getLocation(), parameterName.getParameter()); | ||
|
|
||
| assertThat(stdOut.toString()).contains("Deleted regional parameter:"); |
There was a problem hiding this comment.
The test method name testDeleteRegionalParameter is not very descriptive. Consider renaming it to something more specific, such as testDeleteRegionalParameter_success or testDeleteRegionalParameter_parameterIsDeleted to clearly indicate what the test is verifying.
| @Test | |
| public void testDeleteRegionalParameter() throws IOException { | |
| ParameterName parameterName = TEST_PARAMETER_NAME_TO_DELETE; | |
| DeleteRegionalParam.deleteRegionalParam( | |
| parameterName.getProject(), parameterName.getLocation(), parameterName.getParameter()); | |
| assertThat(stdOut.toString()).contains("Deleted regional parameter:"); | |
| @Test | |
| public void testDeleteRegionalParameter_success() throws IOException { |
Description
Added samples for deleting parameter, enabling, disabling and deleting parameter version using Parameter manager SDK
Sample List (regional):
delete-parameter
disable-parameters-version
enable-disabled-parameters-version
delete-parameter-version
Added required tests for it.
Checklist
pom.xmlparent set to latestshared-configurationGOOGLE_CLOUD_PROJECTto be set.mvn clean verifyrequiredmvn -P lint checkstyle:checkrequiredmvn -P lint clean compile pmd:cpd-check spotbugs:checkadvisory only