feat(ecs-patterns): allow specifying task role for QueueProcessingFargateService and QueueProcessingEc2Service#37320
Conversation
…gateService and QueueProcessingEc2Service Add a taskRole property to QueueProcessingServiceBaseProps that is passed to the internally created task definition. This allows users to provide a custom IAM role for the task without having to create a full task definition. Closes aws#16297
github-actions
bot
added
effort/small
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Features must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
✅ A exemption request has been requested. Please wait for a maintainer's review.
…QueueProcessingFargateService
|
Exemption Request: Integration test snapshot cannot be generated locally because the |
aws-cdk-automation
added
the
pr-linter/exemption-requested
aws-cdk-automation
temporarily deployed
to
automation
GitHub Actions
Inactive
aws-cdk-automation
temporarily deployed
to
automation
GitHub Actions
Inactive
aws-cdk-automation
added
the
pr/needs-further-review
aws-cdk-automation
temporarily deployed
to
automation
GitHub Actions
Inactive
aws-cdk-automation
temporarily deployed
to
automation
GitHub Actions
Inactive
There was a problem hiding this comment.
Hi @syukawa-gh
Added some comments. Once the changes are in place, this could be merged.
Related to the integration test exemption, even though the code change is straightforward, we want to verify the deployment behavior is correct (e.g., the task role is properly attached to the task definition at deploy time). Please add an integ tests that deploys a QueueProcessingFargateService and QueueProcessingEc2Service with a custom taskRole and asserts the resulting task definition has the expected role ARN.
| const stack = new cdk.Stack(); | ||
| const vpc = new ec2.Vpc(stack, 'VPC'); | ||
| const cluster = new ecs.Cluster(stack, 'Cluster', { vpc }); | ||
| const iam = require('../../../aws-iam'); |
There was a problem hiding this comment.
Please add a proper import for this dependency. require is a pattern we dont use in this repo
| /** | ||
| * The role that will be used by the task. | ||
| * | ||
| * Only used when `image` is specified (not when `taskDefinition` is provided). |
There was a problem hiding this comment.
When taskDefinition is provided, the new taskRole prop is silently ignored. The existing code already validates the image + taskDefinition conflict with a clear error, we should also add a similar validation (or expand the existing one) for this new prop
| // Create a Task Definition for the container to start | ||
| this.taskDefinition = new Ec2TaskDefinition(this, 'QueueProcessingTaskDef', { | ||
| family: props.family, | ||
| taskRole: props.taskRole, |
There was a problem hiding this comment.
This will also require proper unit + integration testing
alvazjor
removed
the
pr-linter/exemption-requested
|
Superseded by new PR with review fixes applied (proper import, taskRole+taskDefinition validation, integ test). Rebased on latest main to remove unrelated file changes. |
|
Comments on closed issues and PRs are hard for our team to see. |
4 participants
Add a
taskRoleproperty toQueueProcessingServiceBasePropsthat is passed to the internally created task definition. This allows users to provide a custom IAM role for the task without having to create a full task definition.Closes #16297