Conversation
nborges-aws
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
nborges-aws
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
nborges-aws
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
nborges-aws
had a problem deploying
to
auto-approve
GitHub Actions
Failure
✅ No Breaking Changes DetectedNo public API breaking changes found in this PR. |
Hweinstock
left a comment
Hweinstock
left a comment
There was a problem hiding this comment.
Nice! I like the client. Few questions on the integ tests, but otherwise looks good.
| ) | ||
| except Exception as e: | ||
| print(f"Failed to delete policy {policy_id}: {e}") | ||
| # Wait for policy deletes to complete before deleting engines |
There was a problem hiding this comment.
should we poll here instead of waiting?
There was a problem hiding this comment.
Yep. I added the short delay just as a safeguard, but delete_and_wait method is more robust. I'll change to use that
| policyEngineId=self.engine_ids[0], | ||
| description={"optionalValue": "updated by integ test"}, | ||
| ) | ||
| assert updated["status"] == "ACTIVE" |
There was a problem hiding this comment.
should we verify that the policy actually updated?
There was a problem hiding this comment.
Definitely. I'll assert against the description
| cls.policy_ids = [] | ||
| sts = boto3.client("sts", region_name=cls.region) | ||
| account_id = sts.get_caller_identity()["Account"] | ||
| cls.gateway_resource_arn = f"arn:aws:bedrock-agentcore:{cls.region}:{account_id}:gateway/*" |
There was a problem hiding this comment.
is this used anywhere?
There was a problem hiding this comment.
Good catch. I previously needed this in the cedar statement for one of the tests, but found a way around it. Leftover artifact that can be removed.
| logger.info("Policy engine '%s' already exists, looking up...", name) | ||
| engine_id = self._find_policy_engine_by_name(name) | ||
| if not engine_id: | ||
| raise |
There was a problem hiding this comment.
nit: if a gateway exists, but the lookup fails, the user sees a conflict exception here. Not sure how this could happen, but we might want a better error to avoid confusing customers.
There was a problem hiding this comment.
Will update error messaging here
|
|
||
|
|
||
| @pytest.mark.integration | ||
| class TestPolicyEngineClient: |
There was a problem hiding this comment.
should we also test create_or_get methods?
There was a problem hiding this comment.
Yep, I'll add these
nborges-aws
had a problem deploying
to
auto-approve
GitHub Actions
Failure
nborges-aws
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
nborges-aws
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
nborges-aws
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
2 participants
Issue #, if available: Issue #392
Description of changes:
__getattr__passthrough*_and_waitpolling methods using shared wait_until/wait_until_deleted utilities:generate_policy_asset_and_wait— starts NL→Cedar generation, polls until complete, optionally fetches generated assetsgenerate_and_create_policy— end-to-end flow: generate asset + create policy from it in one callcreate_policy_from_generation_asset— creates a policy from a specific generation assetcreate_or_get_policy_engine / create_or_get_policy— idempotent creates (handles ConflictException by finding existing resource)*_and_waitmethods accept WaitConfig for configurable polling behavior and applyconvert_kwargsfor snake_case supportTest plan:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.