feat(ec2): blockDevice property #4781

nmussy · 2019-10-31T10:05:10Z

Add blockDevice property to EC2 Instance,
using previous aws-autoscaling's previous implementation

BREAKING CHANGES:

Move BlockDevice* classes, interfaces and enums to from aws-autoscaling package to aws-ec2

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

mergify · 2019-10-31T10:05:47Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

nmussy · 2019-10-31T10:07:24Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

@@ -938,162 +922,6 @@ export interface MetricTargetTrackingProps extends BaseTargetTrackingProps {
  readonly targetValue: number;
 }

-export interface BlockDevice {


I'm not thrilled about this breaking change, but I couldn't find a way to cleanly deprecate asg.BlockDevice, as it would require the following type for AutoScalingGroupProps:

readonly blockDevices?: (asg.BlockDevice | ec2.BlockDevice)[];

Couldn't asg.BlockDevice just be more-or-less a copy of ec2.BlockDevice?

I agree, not ideal, but at least not a breaking change.

aws-cdk-automation · 2019-10-31T10:11:55Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-31T10:24:05Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-31T10:48:01Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-31T11:24:45Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr

I'd rather you copy/paste the structs. Maybe put them in a separate file with a note to say they're copies that exist for backwards compatibility.

For bonus points, @deprecate the existing types and property and maybe add a property called something like ec2BlockDevices which is the appropriate type (imported from ec2).

As long as we can find ways to not break customers, I want to do that. Let's not take our contract lightly.

Pull request has been modified.

nmussy · 2019-11-05T09:30:56Z

@rix0rrr I ended up inheriting everything but the EbsDeviceVolumeType enum. I'm not a fan of ec2BlockDevices though. Would you mind keeping this as is, without deprecations?

aws-cdk-automation · 2019-11-05T09:35:01Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-05T09:37:22Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

nmussy · 2019-11-05T09:39:14Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

+export interface EbsDeviceOptions extends ec2.EbsDeviceOptions {}
+export interface EbsDeviceOptionsBase extends ec2.EbsDeviceOptionsBase {}
+export interface EbsDeviceProps extends ec2.EbsDeviceProps {}
+export interface EbsDeviceSnapshotOptions extends ec2.EbsDeviceSnapshotOptions {}


There's an issue with the interfaces. JSII complains if I attempt to export them:

@aws-cdk/aws-autoscaling: error: [awslint:no-unused-type:@aws-cdk/aws-autoscaling.EbsDeviceOptions] type or enum is not used by exported classes (declared at lib/auto-scaling-group.ts:926) @aws-cdk/aws-autoscaling: error: [awslint:no-unused-type:@aws-cdk/aws-autoscaling.EbsDeviceOptionsBase] type or enum is not used by exported classes (declared at lib/auto-scaling-group.ts:927) @aws-cdk/aws-autoscaling: error: [awslint:no-unused-type:@aws-cdk/aws-autoscaling.EbsDeviceProps] type or enum is not used by exported classes (declared at lib/auto-scaling-group.ts:928) @aws-cdk/aws-autoscaling: error: [awslint:no-unused-type:@aws-cdk/aws-autoscaling.EbsDeviceSnapshotOptions] type or enum is not used by exported classes (declared at lib/auto-scaling-group.ts:929) @aws-cdk/aws-autoscaling: error: [awslint:no-unused-type:@aws-cdk/aws-autoscaling.EbsDeviceVolumeType] type or enum is not used by exported classes (declared at lib/auto-scaling-group.ts:933)

Copy-pasta time?

Yes. asg.BlockDevice doesn't use asg.EbsDeviceOptions, it uses ec2.EbsDeviceOptions, and so on.

I agree it's not great, but it's practical, and importantly: non-breaking.

rix0rrr · 2019-11-06T14:46:15Z

Would you mind keeping this as is, without deprecations?

I would not mind at all.

mergify · 2019-12-16T12:34:15Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:01:24Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:53:30Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:39:15Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:52:59Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T15:18:16Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

rix0rrr · 2019-12-27T13:24:43Z

Superseded by new PR.

@nmussy

Add `blockDevice` property to EC2 `Instance`, based on `aws-autoscaling`s previous implementation. We can't unify the two implementations, as that will break API compatibility. Hence, the two libraries have different types for specifying block devices. Continuation of old PR by @nmussy for which the source branch has disappeared. Fixes #4773, closes #4781.

Jimmy Gaussen added 2 commits October 31, 2019 11:01

feat(ec2): blockDevice property

dd3d349

unstaged files

bc46cf2

nmussy requested a review from rix0rrr as a code owner October 31, 2019 10:05

nmussy commented Oct 31, 2019

View reviewed changes

chore: doc fixes

9cb186c

chore: more doc fixes

ad0f214

fix: add breaking changes

3dbc018

SomayaB assigned rix0rrr Oct 31, 2019

rix0rrr previously requested changes Nov 1, 2019

View reviewed changes

Jimmy Gaussen added 2 commits November 5, 2019 09:55

Merge remote-tracking branch 'upstream/master' into 4773

8127688

fix: remove breaking changes

8511de7

chore: reset allowed-breaking-changes

775018c

nmussy commented Nov 5, 2019

View reviewed changes

rix0rrr mentioned this pull request Dec 27, 2019

feat(ec2): support block devices for Instance #5567

Merged

rix0rrr closed this Dec 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ec2): blockDevice property #4781

feat(ec2): blockDevice property #4781

nmussy commented Oct 31, 2019

mergify bot commented Oct 31, 2019

nmussy Oct 31, 2019

rix0rrr Nov 1, 2019

aws-cdk-automation commented Oct 31, 2019

aws-cdk-automation commented Oct 31, 2019

aws-cdk-automation commented Oct 31, 2019

aws-cdk-automation commented Oct 31, 2019

rix0rrr left a comment

nmussy commented Nov 5, 2019

aws-cdk-automation commented Nov 5, 2019

aws-cdk-automation commented Nov 5, 2019

nmussy Nov 5, 2019

rix0rrr Nov 6, 2019

rix0rrr commented Nov 6, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

rix0rrr commented Dec 27, 2019

feat(ec2): blockDevice property #4781

feat(ec2): blockDevice property #4781

Conversation

nmussy commented Oct 31, 2019

mergify bot commented Oct 31, 2019

nmussy Oct 31, 2019

Choose a reason for hiding this comment

rix0rrr Nov 1, 2019

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 31, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 31, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 31, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 31, 2019

AWS CodeBuild CI Report

rix0rrr left a comment

Choose a reason for hiding this comment

nmussy commented Nov 5, 2019

aws-cdk-automation commented Nov 5, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 5, 2019

AWS CodeBuild CI Report

nmussy Nov 5, 2019

Choose a reason for hiding this comment

rix0rrr Nov 6, 2019

Choose a reason for hiding this comment

rix0rrr commented Nov 6, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

rix0rrr commented Dec 27, 2019