-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(events-targets): ecs:TagResource permission #28898
Conversation
@@ -0,0 +1,3 @@ | |||
FROM --platform=linux/amd64 public.ecr.aws/docker/library/python:3.12 | |||
ADD index.py . | |||
CMD [ "python3", "./index.py" ] |
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 image needed to be changed. it was using a lambda image which resulted in the following:
entrypoint requires the handler name to be the first argument
.
@ConnorRobertson Any feedback on this one? |
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.
Hi @msambol, apologies for the delay to get around to this. The changes look good to me, just left one comment about accessing region
.
Btw I just realized you're the one behind the Data structures and algorithms in X minutes videos, I'm a big fan!
Thank you for the kind words! 😄 Much appreciated. Good catch on cluster region. I updated the PR. |
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.
Hi @msambol, thanks for the code change (also a big fan 😁), just a small question but otherwise looks good.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…#29633) ### Issue # (if applicable) Closes #29610 Related to #28898 ### Reason for this change Previously the AWS partition was hardcoded so it would not work for other special partitions. ### Description of changes Replace hardcoded `aws` partition to be dynamically added with `this.cluster.stack.partition`. ### Description of how you validated changes Ran integ tests suite, no changes. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
I enabled the following:
aws ecs put-account-setting-default --name tagResourceAuthorization --value on
And then confirmed the task completes successfully.
Closes #28854.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license