feat(secretmanager): Adding tags samples #13745
Conversation
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 expands the Google Cloud Secret Manager Python samples by adding new code examples for managing resource tags. Specifically, it provides utilities to detach existing tag bindings from secrets and to list all tags currently associated with a secret. These new samples cater to both global and regional secret configurations, offering developers clear guidance on how to programmatically interact with the tagging features of Secret Manager. 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 introduces new samples for managing tags on secrets in Secret Manager, for both global and regional secrets. The changes include scripts for listing and detaching tag bindings, along with corresponding tests. My review focuses on improving code clarity, correctness of documentation, and consistency in testing. I've suggested refactoring some loops to be more idiomatic, correcting misleading docstrings, and enhancing a test case to be more robust.
| location_id (str): Region where the secret should be stored (e.g., "us-central1") | ||
| secret_id (str): ID of the secret to create | ||
| Example: | ||
| # Create a regional secret with a customer-managed encryption key | ||
| list_regional_secret_tag_bindings( | ||
| "my-project", | ||
| "my-regional-secret-with-cmek", | ||
| "us-central1", | ||
| ) |
There was a problem hiding this comment.
The docstring contains several inaccuracies that could confuse users:
- The description for
location_idimplies creation (should be stored), but the function is for listing. - The description for
secret_idsaysID of the secret to create, which is incorrect. - The example comment is misleadingly about creating a secret.
- The arguments in the example call for
location_idandsecret_idare swapped.
This suggestion corrects these issues.
| location_id (str): Region where the secret should be stored (e.g., "us-central1") | |
| secret_id (str): ID of the secret to create | |
| Example: | |
| # Create a regional secret with a customer-managed encryption key | |
| list_regional_secret_tag_bindings( | |
| "my-project", | |
| "my-regional-secret-with-cmek", | |
| "us-central1", | |
| ) | |
| location_id (str): Region where the secret is stored (e.g., "us-central1") | |
| secret_id (str): ID of the secret to list tag bindings for | |
| Example: | |
| # List tag bindings for a regional secret | |
| list_regional_secret_tag_bindings( | |
| "my-project", | |
| "us-central1", | |
| "my-regional-secret", | |
| ) |
| binding_name = None | ||
| request = resourcemanager_v3.ListTagBindingsRequest(parent=parent) | ||
|
|
||
| for binding in client.list_tag_bindings(request=request): | ||
| if binding.tag_value == tag_value: | ||
| binding_name = binding.name | ||
| break |
There was a problem hiding this comment.
The loop to find the binding name can be made more concise and idiomatic by using a generator expression with next().
request = resourcemanager_v3.ListTagBindingsRequest(parent=parent)
binding_name = next(
(b.name for b in client.list_tag_bindings(request=request) if b.tag_value == tag_value),
None,
)There was a problem hiding this comment.
For loop version is more readable to user.
| parent = f"//secretmanager.googleapis.com/{secret_name}" | ||
|
|
||
| # List all tag bindings. | ||
| tag_bindings = [] |
There was a problem hiding this comment.
The tag_bindings list is initialized but its value is never used. It can be removed to simplify the code.
| print(f"Tag bindings for {secret_name}:") | ||
| for binding in bindings: | ||
| print(f"- Tag Value: {binding.tag_value}") | ||
| tag_bindings.append(binding) |
There was a problem hiding this comment.
Since the tag_bindings list is not used, this append operation is unnecessary and can be removed.
| binding_name = None | ||
| request = resourcemanager_v3.ListTagBindingsRequest(parent=parent) | ||
| tag_bindings = tag_bindings_client.list_tag_bindings(request=request) | ||
|
|
||
| for binding in tag_bindings: | ||
| if binding.tag_value == tag_value: | ||
| binding_name = binding.name | ||
| break |
There was a problem hiding this comment.
This loop to find the binding name can be simplified and made more Pythonic by using a generator expression with next().
request = resourcemanager_v3.ListTagBindingsRequest(parent=parent)
tag_bindings = tag_bindings_client.list_tag_bindings(request=request)
binding_name = next(
(binding.name for binding in tag_bindings if binding.tag_value == tag_value),
None,
)There was a problem hiding this comment.
For loop version is more readable to user.
| print(f"Tag bindings for {name}:") | ||
| count = 0 | ||
|
|
||
| # Use the list_tag_bindings method to get all tag bindings | ||
| request = resourcemanager_v3.ListTagBindingsRequest(parent=parent) | ||
| tag_bindings = tag_bindings_client.list_tag_bindings(request=request) | ||
|
|
||
| # Iterate through the results | ||
| for binding in tag_bindings: | ||
| print(f"- Tag Value: {binding.tag_value}") | ||
| count += 1 | ||
|
|
||
| if count == 0: | ||
| print(f"No tag bindings found for {name}.") |
There was a problem hiding this comment.
The header Tag bindings for {name}: is printed even if no bindings are found. It would provide a better user experience to only print the header when there are bindings to display.
| print(f"Tag bindings for {name}:") | |
| count = 0 | |
| # Use the list_tag_bindings method to get all tag bindings | |
| request = resourcemanager_v3.ListTagBindingsRequest(parent=parent) | |
| tag_bindings = tag_bindings_client.list_tag_bindings(request=request) | |
| # Iterate through the results | |
| for binding in tag_bindings: | |
| print(f"- Tag Value: {binding.tag_value}") | |
| count += 1 | |
| if count == 0: | |
| print(f"No tag bindings found for {name}.") | |
| count = 0 | |
| # Use the list_tag_bindings method to get all tag bindings | |
| request = resourcemanager_v3.ListTagBindingsRequest(parent=parent) | |
| tag_bindings = tag_bindings_client.list_tag_bindings(request=request) | |
| # Iterate through the results | |
| for binding in tag_bindings: | |
| if count == 0: | |
| print(f"Tag bindings for {name}:") | |
| print(f"- Tag Value: {binding.tag_value}") | |
| count += 1 | |
| if count == 0: | |
| print(f"No tag bindings found for {name}.") |
There was a problem hiding this comment.
It is good to print and display empty tag values instead of printing None.
| def test_detach_tag( | ||
| project_id: str, | ||
| tag_key_and_tag_value: Tuple[str, str], | ||
| secret_id: str, | ||
| ) -> None: | ||
| """Test detaching a tag from a secret.""" | ||
| # Get the tag value from the fixture | ||
|
|
||
| _, tag_value = tag_key_and_tag_value | ||
|
|
||
| # First bind the tag to the secret | ||
|
|
||
| bind_tags_to_secret(project_id, secret_id, tag_value) | ||
| secret_name = f"projects/{project_id}/secrets/{secret_id}" | ||
|
|
||
| # Now detach the tag | ||
|
|
||
| detach_tag(project_id, secret_id, tag_value) | ||
|
|
||
| client = resourcemanager_v3.TagBindingsClient() | ||
| parent = f"//secretmanager.googleapis.com/{secret_name}" | ||
| request = resourcemanager_v3.ListTagBindingsRequest(parent=parent) | ||
|
|
||
| # Check that none of the bindings contain our tag value | ||
|
|
||
| tag_found = False | ||
| for binding in client.list_tag_bindings(request=request): | ||
| if binding.tag_value == tag_value: | ||
| tag_found = True | ||
| break | ||
| assert ( | ||
| not tag_found | ||
| ), f"Tag value {tag_value} should have been detached but was found" |
There was a problem hiding this comment.
For consistency with test_detach_regional_tag, this test should also verify the output message from the detach_tag function. This requires adding the capsys fixture to the test function and asserting that the success message is present in the output.
def test_detach_tag(
capsys: pytest.LogCaptureFixture,
project_id: str,
tag_key_and_tag_value: Tuple[str, str],
secret_id: str,
) -> None:
"""Test detaching a tag from a secret."""
# Get the tag value from the fixture
_, tag_value = tag_key_and_tag_value
# First bind the tag to the secret
bind_tags_to_secret(project_id, secret_id, tag_value)
secret_name = f"projects/{project_id}/secrets/{secret_id}"
# Now detach the tag
detach_tag(project_id, secret_id, tag_value)
out, _ = capsys.readouterr()
assert "Detached tag value" in out
client = resourcemanager_v3.TagBindingsClient()
parent = f"//secretmanager.googleapis.com/{secret_name}"
request = resourcemanager_v3.ListTagBindingsRequest(parent=parent)
# Check that none of the bindings contain our tag value
tag_found = False
for binding in client.list_tag_bindings(request=request):
if binding.tag_value == tag_value:
tag_found = True
break
assert (
not tag_found
), f"Tag value {tag_value} should have been detached but was found"1 participant
Adding tags samples
Fixes #
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
nox -s py-3.9(see Test Environment Setup)nox -s lint(see Test Environment Setup)