-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(aws-ecs): add ECS/Fargate QueueWorkerService constructs #2568
Conversation
Addressed all PR comments, and updated revision. |
packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-action.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts
Outdated
Show resolved
Hide resolved
memoryLimitMiB: props.memoryLimitMiB, | ||
memoryReservationMiB: props.memoryReservationMiB, | ||
cpu: props.cpu, | ||
command: props.command !== undefined ? cdk.Fn.split(",", props.command) : undefined, |
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.
Really, split on comma? Doesn't seem idiomatic to have to pass "my,command,with,args"
.
Why not define command as command?: string[]
and let the caller worry on how to make a list of strings?
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 was previously discussed: #2363 (comment), and @clareliguori had mentioned that the console currently supports comma delimited strings as command inputs.
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.
Well, I have to disagree with Clare there, sorry.
If there is structure to be had, I'd rather maintain it. I feel like the console decision to split on commas is because a list of input boxes are a more unfriendly UI than a single input box (for copy/pasting etc). We have no such problem though, everything is text.
If a user REALLY REALLY wants to split a comma-separated string, they can type:
"my,command,with,args".split(",")
themselves.
IOW, string[]
leaves all options open, whereas an implicitly ,
-split string precludes using commas in the command, for example.
I'm going to have to insist you change this back.
Modulo the type of |
…ing implementation to add ScalingTargetId and default AdjustmentType
Update command to string[]. Rebasing again, to try to resolve these build issues. |
Resolved build failures. @rix0rrr can you merge this in? I'll refactor this into the new module as a separate PR |
Implement an ECS and Fargate Queue worker that creates an sqs queue and fixes Step Scaling implementation to add ScalingTargetId and default AdjustmentType
Fixes #2396
Pull Request Checklist
design
folderBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.