fix: add subnets prop to GuScheduledEcsTask

[marsavar]

What does this change?

Adds a new optional prop to this pattern.

When attempting to synth a template, the command fails with this error message:

 There are no 'Private' subnet groups in this VPC.

This is expected - it's not possible to know what kinds of subnets are in a given VPC at compile time if a context file isn't present. Passing in this prop should address the issue and make synth work.

How to test

I made this change to my local version of GuCDK and updated package.json to point to the version of GuCDK in my filesystem rather than the one from npm, so that I was able to test this change in my stack. After passing in private subnets obtained with GuVpc.subnetsFromParameter I was able to synth without errors.

How can we measure success?

We don't have to commit a context file to version control when using this pattern.

Checklist

• I have listed any breaking changes, along with a migration path ¹
• I have updated the documentation as required for the described changes ²

Footnotes

1. Consider whether this is something that will mean changes to projects that have already been migrated, or to the CDK CLI tool. If changes are required, consider adding a checklist here and/or linking to related PRs. ↩

2. If you are adding a new construct or pattern, has new documentation been added? If you are amending defaults or changing behaviour, are the existing docs still valid? ↩

[philmcmahon]

Thanks for sorting this @marsavar ! Looks like an omission on my side

[nicl]

Thanks!

[akash1810]

I think we can be more opinionated here, and always place the task in the private subnet (similar to how we implement EC2 apps). WDYT?

[marsavar]

I thought about it but wondered whether there were any use cases where a consumer would want the task in the public subnet. Couldn't really come up with any.
I think your suggestion makes a lot of sense, especially since we have a precedent in the way we do EC2 apps. Will default to private subnets then, and document it.

[marsavar]

Were you imagining something like this?

const privateSubnets = GuVpc.subnetsFromParameter(scope, { type: SubnetType.PRIVATE, app });

const task = new EcsRunTask(scope, `${id}-task`, {
    cluster,
    subnets: { subnets: privateSubnets },
    ...
}

This would effectively make the subnets prop redundant, since we're defining the subnets in the pattern constructor.

[akash1810]

That would work. If you wanted to keep the subnets prop¹, would recommend providing the destructuring with default values pattern (see taskTimeoutInMinutes above, as an example).

Footnotes

1. Would recommend documenting the existence of this prop ↩

[marsavar]

All done, thanks. Have also added a couple of tests.
Let me know if I need to do anything else before I merge! :-)

[akash1810]

Do we know where these new mappings originate from? Feels unrelated to the change we're making in this PR, so it's surprising to see.

[marsavar]

Not quite sure - I think I deleted the snapshot file while working on this, and rerunning the test generated these mappings. 🫠

[akash1810]

Worth noting that service-catalogue takes a slightly different approach to ECS than the GuCDK library, namely it:

• Places all tasks in one cluster (GuCDK creates a cluster per task)
• Implements a VPC work-around

Appreciate this isn't strictly related to this PR, but worth considering.