Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(ecs-patterns): add ecs exec support #18663

Merged
merged 16 commits into from
Jun 13, 2022
Merged

feat(ecs-patterns): add ecs exec support #18663

merged 16 commits into from
Jun 13, 2022

Conversation

LukvonStrom
Copy link
Contributor

@LukvonStrom LukvonStrom commented Jan 26, 2022 •
edited by peterwoodworth
Loading

Fixes #15769, #15197
Supersedes #15497 by implementing the change for all patterns.
This PR implements support for ECS Exec in all ecs-patterns services.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Jan 26, 2022

@LukvonStrom LukvonStrom changed the title feat(ecs-patterns): Add ECS Exec support feat(aws-ecs-patterns): Add ECS Exec support Jan 26, 2022
@LukvonStrom
Copy link
Contributor Author

Apparently the PR Triage assigner is broken, so tagging @madeline-k for PR review and @peterwoodworth bc of commit de45453, which apparently broke the triage

@peterwoodworth
Copy link
Contributor

Thank you very much! @LukvonStrom

@peterwoodworth
Copy link
Contributor

Hope you don't mind me editing your body real quick to test if the automation is working now 😉

@github-actions github-actions bot added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Jan 26, 2022
@peterwoodworth
Copy link
Contributor

killer. Thanks again!

@LukvonStrom
Copy link
Contributor Author

no problem, glad I could help @peterwoodworth :)

@LukvonStrom
Copy link
Contributor Author

hi @madeline-k - I ran in several problems while using ecs, which would be way easier to debug, if this was merged - Is this PR ok for you?

@madeline-k
Copy link
Contributor

madeline-k commented Feb 1, 2022 •
edited
Loading

Hey @LukvonStrom, thanks for your contribution! I haven't reviewed this yet. But will do my best to look at it soon. Looks like this PR supersedes #15497, so closing that one.

@madeline-k madeline-k mentioned this pull request Feb 1, 2022
Closed
@LukvonStrom
Copy link
Contributor Author

hi @madeline-k any news on this front? :) - I'm planning on using this feature in a big customer project

@rix0rrr rix0rrr added feature-request A feature should be added or improved. p2 and removed p2 feature-request A feature should be added or improved. @aws-cdk/aws-ecs-patterns Related to ecs-patterns library labels Mar 4, 2022
@LukvonStrom LukvonStrom mentioned this pull request Apr 23, 2022
Closed
4 tasks
@github-actions
Copy link

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@LukvonStrom
Copy link
Contributor Author

Hi @madeline-k can we please merge now? This PR is now more than three months old.

madeline-k
madeline-k previously requested changes May 2, 2022
View reviewed changes
Copy link
Contributor

@madeline-k madeline-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for authoring this PR @LukvonStrom! It looks good, I just have a few minor comments.

The unit tests also need tests to verify that the EnableExecuteCommand property is absent from CloudFormation templates when it is false or not provided.

packages/@aws-cdk/aws-ecs-patterns/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs-patterns/lib/base/network-multiple-target-groups-service-base.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs-patterns/test/ec2/l3s-v2.test.ts Show resolved Hide resolved
packages/@aws-cdk/aws-ecs-patterns/test/ec2/l3s-v2.test.ts Show resolved Hide resolved
packages/@aws-cdk/aws-ecs-patterns/test/ec2/l3s-v2.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs-patterns/test/ec2/queue-processing-ecs-service.test.ts Show resolved Hide resolved
packages/@aws-cdk/aws-ecs-patterns/test/ec2/queue-processing-ecs-service.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs-patterns/test/fargate/load-balanced-fargate-service-v2.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs-patterns/test/fargate/load-balanced-fargate-service-v2.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs-patterns/README.md Show resolved Hide resolved
@LukvonStrom @madeline-k
Apply suggestions from code review by @madeline-k
d658e2d 
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
@github-actions
Copy link

github-actions bot commented May 7, 2022

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

4 similar comments
@github-actions
Copy link

github-actions bot commented May 8, 2022

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@github-actions
Copy link

github-actions bot commented May 9, 2022

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@github-actions
Copy link

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@github-actions
Copy link

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@LukvonStrom LukvonStrom changed the base branch from master to v2-main May 11, 2022 19:04
@github-actions github-actions bot added the effort/small Small work item – less than a day of effort label May 11, 2022
@Testare
Copy link

Testare commented May 27, 2022

Thank you for working on this! I hope this can be merged soon

@alickwong
Copy link

looking forward to this feature

@LukvonStrom
Copy link
Contributor Author

cc/ @madeline-k - there is high interest in getting this merged

@fsmanuel
Copy link

fsmanuel commented Jun 10, 2022 •
edited
Loading

@madeline-k Can I help to get this merged? Looking forward to use the ECS Exec feature!

@TheRealAmazonKendra TheRealAmazonKendra changed the title feat(aws-ecs-patterns): Add ECS Exec support feat(ecs-patterns): add ecs exec support Jun 13, 2022
@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2022

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: c07d87b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 23ee450 into aws:main Jun 13, 2022
@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2022

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).

@TheRealAmazonKendra TheRealAmazonKendra mentioned this pull request Jun 14, 2022
Closed
daschaa pushed a commit to daschaa/aws-cdk that referenced this pull request Jul 9, 2022
@LukvonStrom @daschaa
feat(ecs-patterns): add ecs exec support (aws#18663)
a3b46a9 
Fixes aws#15769, aws#15197
Supersedes aws#15497 by implementing the change for all patterns.
This PR implements support for ECS Exec in all ecs-patterns services.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@factoidforrest
Copy link

This is not in 1.188.0 as of today. Is everything okay? Where is it? Did this get rolled back somewhere?

@varju
Copy link
Contributor

varju commented Jan 16, 2023

This is not in 1.188.0 as of today. Is everything okay? Where is it? Did this get rolled back somewhere?

From right below your comment in the PR:
Screenshot 2023-01-16 at 12 10 28 PM

2.x is where all the new feature work happens.

@factoidforrest
Copy link

Sorry, release of what? Is the package no longer
@aws-cdk/aws-ecs-patterns?

@varju
Copy link
Contributor

varju commented Jan 16, 2023

The CDK package structure changed radically for v2. See https://docs.aws.amazon.com/cdk/v2/guide/migrating-v2.html

@factoidforrest
Copy link

Oh...wow. ok. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheRealAmazonKendra TheRealAmazonKendra TheRealAmazonKendra approved these changes

@madeline-k madeline-k Awaiting requested review from madeline-k

Assignees

@madeline-k madeline-k

Labels
effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(aws-ecs-patterns): Add enableExecuteCommand configuration property ApplicationLoadBalancedFargateService