feat(ecs): ECS optimized Windows images #3376

nmussy · 2019-07-22T09:34:00Z

deprecate EcsOptimizedAmi for EcsOptimizedImage
constructor(props) replaced by strongly typed static methods
Windows AMI support
deprecate EcsOptimizedAmi, EcsOptimizedAmiProps
will require changes on v2 shipment #3398

Please read the contribution guidelines and follow the pull-request checklist.

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

nmussy · 2019-07-22T09:35:05Z

packages/@aws-cdk/aws-ecs/lib/cluster.ts

+   *
+   * @default none, uses Linux generation
+   */
+  readonly windowsVersion?: WindowsOptimizedVersion;


Ideally, the generation and windowsVersion properties would be merged, but I didn't think it was worth the refactoring and breaking change.

Maybe we can @deprecate generation in favor of amazonLinuxGeneration (still support generation for backwards compat and in the next MV we will remove)

Thoughts? Maybe in a subsequent PR? At least update #3398 with this.

Sorry, I didn't see your previous comment. I can add it to this PR

Shouldn't we just replace it with an image union construct, to have the definition enforce the mutual exclusion?

So what's up with this? Didn't we say this is gone?

eladb

Looks good.

eladb · 2019-07-23T11:54:09Z

packages/@aws-cdk/aws-ecs/lib/cluster.ts

      } else {
        this.generation = props.generation;
      }
+    } else if (props && props.windowsVersion) {
+      if (this.hwType !== AmiHardwareType.STANDARD) {
+        throw new Error('Windows Server does not support special hardware type. Use Amazon Linux 2 instead');


Remove "use Amazon Linux 2 instead". I am assuming that if a user explicitly specified they wanted Windows, they should probably just not specify the HW type and not use Linux instead... :-)

eladb · 2019-07-23T11:55:08Z

packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts

    });

+    // THEN
+    expect(stack).to(haveResource("AWS::AutoScaling::LaunchConfiguration", {


You should add an expectation on the parameter with the desired SSM path...

Sorry, I don't understand your comment. I reused the existing HW AMI Type test:

aws-cdk/packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts

Lines 252 to 256 in 47e81a6

expect(stack).to(haveResource("AWS::AutoScaling::LaunchConfiguration", {

ImageId: {

Ref: "SsmParameterValueawsserviceecsoptimizedamiamazonlinux2gpurecommendedimageidC96584B6F00A464EAD1953AFF4B05118Parameter"

}

}));

This just shows the ImageId is a reference to a synthesized CloudFormation parameter that is added to your template. You should write your expectation against the default value of the parameter which will contain the SSM parameter name that you want.

What I usually do is expect(stack).toMatch({}) and then I can see which parts of the stack to assert against.

Good call, I was missing an assignation, thanks.

I have an issue with the matching now. I tried changing the MatchStyle, but it doesn't seem to detect when I purposefully change the Default matched value.

Should I use an integration test instead, to match the whole stack?

Yeah, I think there's a bug in the matcher (raise an issue?). What you could do in the meantime is as follows;

// you'll need an "App" const app = new App(); const stack = new Stack(app, 'test'); // ... // then const assembly = app.synth(); const template = assembly.getStack(stack.stackName).template; test.deepEqual(template.Parameters, { /* expectation here */ });

Alright, thanks a lot. Should I also update the existing Linux AMI test?

aws-cdk/packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts

Lines 253 to 257 in 5c01a39

expect(stack).to(haveResource("AWS::AutoScaling::LaunchConfiguration", {

ImageId: {

Ref: "SsmParameterValueawsserviceecsoptimizedamiamazonlinux2gpurecommendedimageidC96584B6F00A464EAD1953AFF4B05118Parameter"

}

}));

If so, should I do it in another PR?

Feel free to add to this PR

Done. Thanks again for the help!

* incomplete test, stack incomplete

eladb · 2019-07-24T11:43:27Z

packages/@aws-cdk/aws-ecs/lib/cluster.ts

+   *
+   * @default none, uses Linux generation
+   */
+  readonly windowsVersion?: WindowsOptimizedVersion;


Thoughts? Maybe in a subsequent PR? At least update #3398 with this.

* EcsOptimizedAmi -> EcsOptimizedAmiStatic * deprecate EcsOptimizedAmi, EcsOptimizedAmiProps

packages/@aws-cdk/aws-ecs/lib/cluster.ts

eladb · 2019-07-25T12:21:45Z

packages/@aws-cdk/aws-ecs/lib/cluster.ts

+   *
+   * @default none, uses Linux generation
+   */
+  readonly windowsVersion?: WindowsOptimizedVersion;


So what's up with this? Didn't we say this is gone?

* EcsOptimizedAmiStatic -> EcsOptimizedImage

feat(ecs): support windows images

eb63ce4

nmussy requested a review from a team as a code owner July 22, 2019 09:34

nmussy commented Jul 22, 2019

View reviewed changes

fix(ecs): throw if both windowsVersion and generation props are set

48780fb

eladb reviewed Jul 23, 2019

View reviewed changes

Jimmy Gaussen and others added 6 commits July 23, 2019 13:57

fix(ecs): remove linux mention in windows hardware error

47e81a6

fix(ecs): fix windowsVersion assignation, update test

e3e5739

* incomplete test, stack incomplete

fix(ecs): windowsVersion test deepEqual

5c01a39

fix(ecs): test linux ami ssm parameter

b1f003b

Merge branch 'master' into feat-windows-ecs

a95a33f

Merge branch 'master' into feat-windows-ecs

f2a8e1d

eladb approved these changes Jul 24, 2019

View reviewed changes

Jimmy Gaussen added 6 commits July 24, 2019 15:20

fix(ecs): remove duplicate core import

0309b87

fix(ecs): tslint

9bc06bf

feat(ecs): EcsOptimizedAmi refactor, use static constructors

12d99eb

* EcsOptimizedAmi -> EcsOptimizedAmiStatic * deprecate EcsOptimizedAmi, EcsOptimizedAmiProps

fix(ecs): remove unused interfaces

450b507

fix(ecs): remove duplicate import

42bfb0b

fix(ecs): fix property ordering

3cbeb5f

eladb reviewed Jul 25, 2019

View reviewed changes

fix(ecs): refactor EcsOptimizedImage

19e874d

* EcsOptimizedAmiStatic -> EcsOptimizedImage

nmussy changed the title ~~feat(ecs): support windows images~~ feat(ecs): EcsOptimizedImage static construct Jul 25, 2019

eladb changed the title ~~feat(ecs): EcsOptimizedImage static construct~~ feat(ecs): support ECS optimized Windows images Jul 25, 2019

eladb changed the title ~~feat(ecs): support ECS optimized Windows images~~ feat(ecs): ECS optimized Windows images Jul 25, 2019

eladb merged commit 6c0bf4a into aws:master Jul 25, 2019

nmussy deleted the feat-windows-ecs branch July 25, 2019 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecs): ECS optimized Windows images #3376

feat(ecs): ECS optimized Windows images #3376

nmussy commented Jul 22, 2019 •

edited

Loading

nmussy Jul 22, 2019

eladb Jul 23, 2019

eladb Jul 24, 2019

nmussy Jul 24, 2019

nmussy Jul 24, 2019

eladb Jul 24, 2019

eladb Jul 25, 2019

eladb left a comment

eladb Jul 23, 2019

nmussy Jul 23, 2019

eladb Jul 23, 2019

nmussy Jul 23, 2019

eladb Jul 23, 2019

nmussy Jul 23, 2019

eladb Jul 23, 2019

nmussy Jul 23, 2019

eladb Jul 23, 2019

nmussy Jul 23, 2019

eladb Jul 24, 2019

eladb Jul 25, 2019

	expect(stack).to(haveResource("AWS::AutoScaling::LaunchConfiguration", {
	ImageId: {
	Ref: "SsmParameterValueawsserviceecsoptimizedamiamazonlinux2gpurecommendedimageidC96584B6F00A464EAD1953AFF4B05118Parameter"
	}
	}));

feat(ecs): ECS optimized Windows images #3376

feat(ecs): ECS optimized Windows images #3376

Conversation

nmussy commented Jul 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmussy commented Jul 22, 2019 •

edited

Loading