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.

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

(aws-ecs): Provide an enableExecuteCommand method #15768

Open
2 tasks
rcollette opened this issue Jul 26, 2021 · 11 comments
Open
2 tasks

(aws-ecs): Provide an enableExecuteCommand method #15768

rcollette opened this issue Jul 26, 2021 · 11 comments
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2

Comments

@rcollette
Copy link
Contributor

rcollette commented Jul 26, 2021

When the enableExecuteCommand property was added, the enableExecuteCommand() method was made private. The logic that handles this property should have also been exposed as a method so that this feature could be enabled subsequent to construction of the construct.

Use Case

For use cases where we do not have direct access to construction of the construct, such as in the case of ApplicationLoadBalancedFargateService.

Proposed Solution

Move all logic related handling of the enableExecuteCommand configuration property into the enableExecuteCommand() method and make that method public.

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@rcollette rcollette added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jul 26, 2021
@github-actions github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Jul 26, 2021
@rcollette rcollette changed the title (aws-ecs): Provide an enableExecutionCommand method (aws-ecs): Provide an enableExecuteCommand method Jul 26, 2021
@ryparker
Copy link
Contributor

ryparker commented Jul 27, 2021

Thanks for creating this and separating it from the other linked issue #15769. 👍🏻

I'm curious to see what @madeline-k says, but for now I will relabel this as a bug and prioritize as p1 since you described that this was previously working as a public method and was changed to private in a recent update.


If anyone is visiting this issue and looking for a quick workaround please take a look at the docs on using escape hatches.

Example of using escape hatches on the ApplicationLoadBalancedFargateService:

const albFargateService = new ApplicationLoadBalancedFargateService(this, 'AlbFargateService', {
	...props
});

// Get the CloudFormation resource
const cfnFargateService = albFargateService.service.node;
cfnFargateService.addPropertyOverride('EnableExecuteCommand', true);

@ryparker ryparker added effort/small Small work item – less than a day of effort p1 bug This issue is a bug. and removed needs-triage This issue or PR still needs to be triaged. feature-request A feature should be added or improved. labels Jul 27, 2021
@rcollette
Copy link
Contributor Author

@ryparker I apologize if I wasn't clear. The method I mentioned was never public. It was "made" in the first place private. So probably not as much a bug (not a breaking one at least)

@ryparker
Copy link
Contributor

ryparker commented Jul 27, 2021

@ryparker I apologize if I wasn't clear. The method I mentioned was never public. It was "made" in the first place private. So probably not as much a bug (not a breaking one at least)

Gotcha thanks for clarifying. In that case i'll mark this as a p2 feature-request which means that we are unable to work on this immediately. We use +1s to help us prioritize our work, and as always we are happy to take contributions if anyone is interested to pick this up and submit a PR (please make sure to follow our contribution guidelines.)
🙏

@ryparker ryparker added feature-request A feature should be added or improved. p2 and removed bug This issue is a bug. p1 labels Jul 27, 2021
@madeline-k
Copy link
Contributor

@ryparker Agree with your triage here. And thanks for providing a workaround!

@madeline-k madeline-k removed their assignment Aug 4, 2021
@LukvonStrom
Copy link
Contributor

To quote from https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-exec.html: "You can't enable ECS Exec for existing tasks. It can only be enabled for new tasks.", so I don't see added benefit with this method

@hoegertn
Copy link
Contributor

Yes, but calling a method on a construct still happens during synth before creating infrastructure. From a CFN perspective, there is no difference between settings props in a constructor or a method call.

@LukvonStrom
Copy link
Contributor

@hoegertn are there any L2 constructs, that expose methods for props, that may be set during creation?

@hoegertn
Copy link
Contributor

Depends on what you count as props. grantXX, addToXXPolicy, allowFrom/To and more I could look up.

@LukvonStrom
Copy link
Contributor

@hoegertn these are side-effect causing methods, which have a legitimate purpose (imho), but I don't see that for this method here

@github-actions
Copy link

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jan 26, 2023
@hoegertn
Copy link
Contributor

Would love if this could happen

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

5 participants