-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix(bedrock-agentcore-alpha): AssetImage artifact causes pipeline sel…
#35982
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
fix(bedrock-agentcore-alpha): AssetImage artifact causes pipeline sel…
#35982
Conversation
…f-mutation on folder change
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.
(This review is outdated)
…f-mutation on folder change
…f-mutation on folder change
AssetImage artifact causes pipeline sel…
|
Exemption Request: |
mazyu36
left a comment
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.
The change itself is LGTM.
Warnings are issued if changes to integ.*.ts are not included.
If you consider it unnecessary, it would be good to add an exemption request.
https://github.com/aws/aws-cdk/blob/main/tools/@aws-cdk/prlint/lint.ts#L394
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Abogical
left a comment
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.
Integ test can be exempted, but a simpler unit test to check the name will be needed. This is just to make sure that we don't change the naming behavior in the future.
Hi - not sure how to test this with a unit test. I'm absolutely no CDK developer (not even a TypeScript expert) - what I saw is that the interally created If still an unit test is needed, I would need detailed advice how to do. Thanks for understanding. |
Pull request has been modified.
…/github.com/sm29105/aws-cdk into fix/bedrock-agentcore-alpha/pipeline-mutate
If you still need help, let us know. |
Abogical
left a comment
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.
See above, unit tests to be added.
Had a look and tried to figure out what to do but failed after 1h now. :( As mentioned I don't know any dev internals of CDK or TypeScript - don't even know how to run tests locally or so. Can please somebody else take over? I would assume it takes 5 minutes for somebody that know what to touch. Thanks for understanding ;) |
|
Ok no problem, I'll keep this PR open so anyone could take over. |
1074a65 to
4dae263
Compare
|
Comments on closed issues and PRs are hard for our team to see. |
fix(bedrock-agentcore-alpha): AssetImage artifact causes pipeline self-mutation on folder content change
Issue # (if applicable)
Closes #35968.
Reason for this change
When using an AssetImage based artifact for the runtime agent a change to the folder contents will change the asset's name which causes a CDK pipeline self-mutation.
This is unexpected behaviour and does not align with similar use-cases like image-based Lambda functions.
Description of changes
Remove MD5 hash component from the image asset's name - like e.g. the case for image assets for AWS Lambda functions
Describe any new or updated permissions being added
None
Description of how you validated changes
Manual test of solution with a company internal pipeline that showed the unexpected behaviour.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license