-
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(ecs): require task pidMode for Linux-based Fargate tasks, not host #30020
Conversation
Appreciate the fix. I see you also added a check for it being a linux os family, which makes sense being the alternative already being present for denying pidmode usage. Though that does change the behavior, where it will now only warn when you set an OS family, I think that is a good thing as it's more consistent to only message on knowns. It does however mean that without an os family you have to will wait until deployment to notice your mistake. If we want a synth level protection, then it would be better to also lock pidmode behind having set an os family. Your current changes mean that if you set |
I like this because it only affects people who are already using the L2 pidMode (which was introduced only a few days ago) and only if they're not specifying an OS family. I'll put together the change. Edit - In fact, because the existing pidMode functionality will fail on deployment (because |
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.
Agree that it's not a breaking change because the deployment would fail with wrong pidMode value before. Left some nit comments
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). |
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). |
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). |
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
Issue # (if applicable)
Closes #29995.
Reason for this change
Only the
task
option is allowed forpidMode
on Linux-based Fargate tasks.Description of changes
This PR builds on the changes introduced in #29670 but fixes the handling of
pidMode
so that it matches the behavior allowed by CloudFormation and described in the AWS User Guide.Description of how you validated changes
Updated the existing tests so that
task
is the only allowablepidMode
setting if a Fargate task's OS is Linux-based.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license