fix(ecs): canContainersAccessInstanceRole is ignored when passed in AsgCapacityProvider constructor

[daschaa]

Fixes #20293

When adding an AsgCapacityProvider the property canContainersAccessInstanceRole is only checked when passed in via the method addAsgCapacityProvider. It is ignored when passing the property via the instantiation of an AsgCapacityProvider. In this PR I added, that if either one way (method or constructor) has got the property set - it is respected in the outcome. For more details please see the issue #20293

I decided not to omit the property on the class level because it would bring in breaking changes.

---

All Submissions:

• Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• This PR adds new unconventional dependencies following the process described here

New Features

• Have you added the new feature to an integration test?
  • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

[gitpod-io]

[corymhall]

@daschaa thanks for the fix!

I'm wondering if it might also be good to throw an error if the user sets canContainersAccessInstanceRole on both and they are different values? Like you explicitly set it to true on one and false on the other, are you sure?

[daschaa]

@corymhall Thanks for the hint! I didn't thought about that case.
An error is a good idea. But this could maybe lead to breaking changes.

Another approach/idea is, that addAsgCapacityProvider could take precedence over the constructor one. If added explicitly to the method, the property is then "overridden".
What do you think?

Pull request has been modified.

[corymhall]

@corymhall Thanks for the hint! I didn't thought about that case. An error is a good idea. But this could maybe lead to breaking changes.

Another approach/idea is, that addAsgCapacityProvider could take precedence over the constructor one. If added explicitly to the method, the property is then "overridden". What do you think?

yeah that's a good call. addAsgCapacityProvider should take precedence to keep backwards compatibility 👍

[daschaa]

@corymhall Cool! I will add a test for that.

[daschaa]

@corymhall I have now added test cases for the precedence. Could you review it again please?
Thanks for your feedback and help! ❤️

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 8787f2a
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[joeflateau]

Thank you @daschaa!

[daschaa]

@joeflateau Thank you for creating the issue! Your finding was 99% of the work ❤️