-
Notifications
You must be signed in to change notification settings - Fork 129
Update DP APIs to support namespace re-design #449
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| """Namespace utilities for data plane API calls.""" | ||
|
|
||
| from typing import Dict, Optional | ||
|
|
||
|
|
||
| def build_namespace_params(namespace: Optional[str] = None, namespace_path: Optional[str] = None) -> Dict[str, str]: | ||
| """Build the namespace kwargs for a data plane API call. | ||
|
|
||
| Exactly one of ``namespace`` (exact match) or ``namespace_path`` | ||
| (hierarchical path prefix) must be provided. Wildcards (``*``) are not | ||
| supported in either field. | ||
|
|
||
| Raises: | ||
| ValueError: if both arguments are provided, neither is provided, or | ||
| the provided value contains a wildcard. | ||
| """ | ||
| if namespace is not None and namespace_path is not None: | ||
| raise ValueError("'namespace' and 'namespace_path' are mutually exclusive.") | ||
| if namespace is None and namespace_path is None: | ||
| raise ValueError("At least one of 'namespace' or 'namespace_path' must be provided.") | ||
|
|
||
| value = namespace if namespace is not None else namespace_path | ||
| if "*" in value: | ||
| raise ValueError("Wildcards (*) are not supported in namespaces.") | ||
|
|
||
| if namespace is not None: | ||
| return {"namespace": namespace} | ||
| return {"namespacePath": namespace_path} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| import pytest | ||
| from botocore.exceptions import ClientError | ||
|
|
||
| from bedrock_agentcore._utils.namespace import build_namespace_params | ||
| from bedrock_agentcore._utils.polling import wait_until, wait_until_deleted | ||
|
|
||
|
|
||
|
|
@@ -112,3 +113,29 @@ def test_timeout(self, _mock_time, _mock_sleep): | |
| poll_fn = Mock(return_value={"status": "DELETING"}) | ||
| with pytest.raises(TimeoutError): | ||
| wait_until_deleted(poll_fn) | ||
|
|
||
|
|
||
| class TestBuildNamespaceParams: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I feel like we have some duplicate coverage of the functionality also tested in tests/bedrock_agentcore/memory/test_client.py. I actually prefer this one since it tests the utils directly, instead of mocking. Does it make sense to consolidate here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, there are a few tests that are similar between these two. My rationale was that the tests in utils are verifying the utility method works as intended, and the tests in the client ensures the end user's experience (user returned |
||
| """Tests for build_namespace_params utility.""" | ||
|
|
||
| def test_namespace_only(self): | ||
| assert build_namespace_params(namespace="/actor/Jane/") == {"namespace": "/actor/Jane/"} | ||
|
|
||
| def test_namespace_path_only(self): | ||
| assert build_namespace_params(namespace_path="/org/team/") == {"namespacePath": "/org/team/"} | ||
|
|
||
| def test_both_raises(self): | ||
| with pytest.raises(ValueError, match="mutually exclusive"): | ||
| build_namespace_params(namespace="/a/", namespace_path="/b/") | ||
|
|
||
| def test_neither_raises(self): | ||
| with pytest.raises(ValueError, match="At least one"): | ||
| build_namespace_params() | ||
|
|
||
| def test_wildcard_in_namespace_raises(self): | ||
| with pytest.raises(ValueError, match="[Ww]ildcard"): | ||
| build_namespace_params(namespace="/actor/*/") | ||
|
|
||
| def test_wildcard_in_namespace_path_raises(self): | ||
| with pytest.raises(ValueError, match="[Ww]ildcard"): | ||
| build_namespace_params(namespace_path="/org/*/team/") | ||
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.
q: is namespace a concept outside of memory? If not, does it make sense for this to live within the memory subfolder?
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.
Yeah this is a memory level concept; I can move utils into the memory folder. Let's do this as a fast-follow after merging open PRs. Given the namespace changes are split over two PRs updating will be easier post-merge.