feat(autoscaling): add support for `InstanceRequirements` property by wafuwafu13 · Pull Request #28464 · aws/aws-cdk

wafuwafu13 · 2023-12-22T11:06:57Z

Basically LaunchTemplateOverrides for L2 construct is missing the InstanceRequirements attribute.

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

…quirements

wafuwafu13 · 2023-12-22T11:07:40Z

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

+  /**
+   * The instance requirements. Amazon EC2 Auto Scaling uses your specified requirements to identify instance types.
+   * Then, it uses your On-Demand and Spot allocation strategies to launch instances from these instance types.
+   * You can specify up to four separate sets of instance requirements per Auto Scaling group.
+   * This is useful for provisioning instances from different Amazon Machine Images (AMIs) in the same Auto Scaling group.
+   * To do this, create the AMIs and create a new launch template for each AMI.
+   * Then, create a compatible set of instance requirements for each launch template.
+   *
+   * If you specify InstanceRequirements, you can't specify InstanceType.
+   *
+   * @default - Do not override instance type
+   */


ref: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-autoscaling-autoscalinggroup-launchtemplateoverrides.html#cfn-autoscaling-autoscalinggroup-launchtemplateoverrides-instancerequirements

wafuwafu13 · 2023-12-22T11:08:28Z

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

+                if (override.instanceType && override.instanceRequirements) {
+                  throw new Error('\'InstanceRequirements\' can\'t be specified with \'InstanceType\'');
+                }


ref:
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-autoscaling-autoscalinggroup-launchtemplateoverrides.html#cfn-autoscaling-autoscalinggroup-launchtemplateoverrides-instancerequirements

If you specify InstanceRequirements, you can't specify InstanceType.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

wafuwafu13 · 2023-12-22T11:15:32Z

Exemption Request

❌ Features must contain a change to a README file.

I add doc comment for instanceRequirements property

❌ Features must contain a change to an integration test file and the resulting snapshot.

I add test at auto-scaling-group.test.ts

wafuwafu13 · 2023-12-22T11:19:07Z

Exemption Request

I add doc comment for instanceRequirements property
I add test at auto-scaling-group.test.ts

wafuwafu13 · 2023-12-22T12:52:40Z

Exemption Request: I add doc comment for instanceRequirements property / I add test at auto-scaling-group.test.ts

sumupitchayan · 2023-12-22T16:27:57Z

@wafuwafu13 please add an integration test or update an existing one

lpizzinidev

Thanks for starting this.
This is still a work in progress as the property needs to be implemented in the L2 construct directly.
Also, the related documentation, unit, and integration tests are required as this is a new feature.

lpizzinidev · 2023-12-22T16:29:33Z

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

+   *
+   * @default - Do not override instance type
+   */
+  readonly instanceRequirements?: CfnAutoScalingGroup.InstanceRequirementsProperty


We'll need to implement the interface in the L2 construct and not make use of the existing one from L1.

Do you mean I should define instanceRequirements in another place?
If so, I would be grateful if you could tell me where to define it.

Nevermind, I got it wrong.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

lpizzinidev

Thanks 👍
I think we should require one of instanceType or instanceRequirements to be specified, feel free to comment if you think otherwise.

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

lpizzinidev · 2023-12-23T18:45:42Z

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

+   *
+   * @default - Do not override instance type
+   */
+  readonly instanceRequirements?: CfnAutoScalingGroup.InstanceRequirementsProperty


Nevermind, I got it wrong.

lpizzinidev · 2023-12-23T18:51:43Z

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

                  throw new Error('Weight must be an integer');
                }
+                if (override.instanceType && override.instanceRequirements) {
+                  throw new Error('\'InstanceRequirements\' can\'t be specified with \'InstanceType\'');


Suggested change

throw new Error('\'InstanceRequirements\' can\'t be specified with \'InstanceType\'');

throw new Error('You can specify either \'instanceRequirements\' or \'instanceType\', not both.');

Message formatting.

lpizzinidev · 2023-12-23T18:57:11Z

packages/aws-cdk-lib/aws-autoscaling/README.md

 });
 ```

+You can specify instance requirements in `InstanceRequirements`:


Suggested change

You can specify instance requirements in `InstanceRequirements`:

You can specify instances requirements with the `instanceRequirements ` property:

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

lpizzinidev · 2023-12-23T19:02:42Z

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

+   * To do this, create the AMIs and create a new launch template for each AMI.
+   * Then, create a compatible set of instance requirements for each launch template.
+   *
+   * If you specify InstanceRequirements, you can't specify InstanceType.


Suggested change

* If you specify InstanceRequirements, you can't specify InstanceType.

* You must specify one of instanceRequirements or instanceType.

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

…nceType

wafuwafu13 · 2023-12-24T03:39:31Z

Thank you for reviewing! I fixed.

mergify · 2023-12-28T14:34:08Z

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

sumupitchayan · 2023-12-28T15:12:41Z

@Mergifyio update

mergify · 2023-12-28T15:12:45Z

update

❌ Mergify doesn't have permission to update

Details

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/close-stale-prs.yml without workflows permission

Pull request has been modified.

aws-cdk-automation · 2023-12-28T15:40:29Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: c6aa56d
Result: SUCCEEDED
Build Logs (available for 30 days)

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

…ws#28464) Closes aws#28393 > Basically [LaunchTemplateOverrides](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_autoscaling.LaunchTemplateOverrides.html) for L2 construct is missing the [InstanceRequirements](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-autoscaling-autoscalinggroup-launchtemplateoverrides.html#cfn-autoscaling-autoscalinggroup-launchtemplateoverrides-instancerequirements) attribute. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --------- Co-authored-by: Sumu Pitchayan <35242245+sumupitchayan@users.noreply.github.com>

feat(autoscaling/LaunchTemplateOverrides): add support for InstanceRe…

cc997ab

…quirements

aws-cdk-automation requested a review from a team December 22, 2023 11:07

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK labels Dec 22, 2023

wafuwafu13 changed the title ~~feat(autoscaling/LaunchTemplateOverrides): add support for InstanceRequirements~~ feat(autoscaling/LaunchTemplateOverrides): add support for InstanceRequirements Dec 22, 2023

wafuwafu13 commented Dec 22, 2023

View reviewed changes

aws-cdk-automation previously requested changes Dec 22, 2023

View reviewed changes

wafuwafu13 changed the title ~~feat(autoscaling/LaunchTemplateOverrides): add support for InstanceRequirements~~ feat(autoscaling): add support for InstanceRequirements property Dec 22, 2023

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Dec 22, 2023

wafuwafu13 requested a review from aws-cdk-automation December 22, 2023 11:20

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Dec 22, 2023

lpizzinidev suggested changes Dec 22, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 22, 2023

wafuwafu13 added 2 commits December 23, 2023 12:48

test(autoscaling): add integration test

3643976

docs(autoscaling): add doc for InstanceRequirements

cf24516

wafuwafu13 requested a review from lpizzinidev December 23, 2023 03:56

lpizzinidev suggested changes Dec 23, 2023

View reviewed changes

fix(autoscaling): should specify either InstanceRequirements or Insta…

f9d3a8e

…nceType

docs(autoscaling): make it more readable

2fb32af

wafuwafu13 requested a review from lpizzinidev December 24, 2023 03:39

sumupitchayan previously approved these changes Dec 28, 2023

View reviewed changes

Merge branch 'main' into asg-instance-requirements

c6aa56d

sumupitchayan approved these changes Dec 28, 2023

View reviewed changes

sumupitchayan merged commit 276e3a6 into aws:main Dec 28, 2023

github-actions bot mentioned this pull request Jan 1, 2024

Monthly PR metrics report - Dec 2023 #28544

Closed

	throw new Error('\'InstanceRequirements\' can\'t be specified with \'InstanceType\'');
	throw new Error('You can specify either \'instanceRequirements\' or \'instanceType\', not both.');

	You can specify instance requirements in `InstanceRequirements`:
	You can specify instances requirements with the `instanceRequirements ` property:

	* If you specify InstanceRequirements, you can't specify InstanceType.
	* You must specify one of instanceRequirements or instanceType.

Conversation

wafuwafu13 commented Dec 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

wafuwafu13 commented Dec 22, 2023

Uh oh!

wafuwafu13 commented Dec 22, 2023

Uh oh!

wafuwafu13 commented Dec 22, 2023

Uh oh!

sumupitchayan commented Dec 22, 2023

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpizzinidev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wafuwafu13 commented Dec 24, 2023

Uh oh!

mergify bot commented Dec 28, 2023

Uh oh!

sumupitchayan commented Dec 28, 2023

Uh oh!

mergify bot commented Dec 28, 2023

❌ Mergify doesn't have permission to update

Uh oh!

aws-cdk-automation commented Dec 28, 2023

AWS CodeBuild CI Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lpizzinidev left a comment •

edited

Loading