feat(autoscaling): add `availabilityZoneDistribution` property to an AutoScalingGroup #32100

ren-yamanashi · 2024-11-12T15:17:53Z

Issue # (if applicable)

n/A

Reason for this change

We can set a availabilityZoneDistribution for an AutoScalingGroup from cloudformation, but this was not supported in the AWS CDK L2 construct.

Description of changes

Add availabilityZoneDistribution property to CommonAutoScalingGroupProps and set it in the CfnAutoScalingGroup.

Description of how you validated changes

Added both unit and integration tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

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.

…vailability-zone-distribution

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

…vailability-zone-distribution

codecov · 2024-11-24T02:40:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.54%. Comparing base (a81eec6) to head (ba90575).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32100   +/-   ##
=======================================
  Coverage   80.54%   80.54%           
=======================================
  Files         106      106           
  Lines        6954     6954           
  Branches     1287     1287           
=======================================
  Hits         5601     5601           
  Misses       1175     1175           
  Partials      178      178

Flag	Coverage Δ
suite.unit	`80.54% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`80.54% <ø> (ø)`

…vailability-zone-distribution

mazyu36

Thank you for the contribution.
I've commented on property design.
If you have any thoughts on that please let me know.

mazyu36 · 2024-12-18T14:28:05Z

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

+   * The instance capacity distribution across Availability Zones.
+   * @default { capacityDistributionStrategy: CapacityDistributionStrategy.BALANCED_BEST_EFFORT }
+   */
+  readonly availabilityZoneDistribution?: AvailabilityZoneDistribution;


IMO, we can reduce a nest like this:

readonly azCapacityDistributionStrategy?: CapacityDistributionStrategy;

In the design docs, fewer nesting is preferred.

mazyu36 · 2024-12-18T14:30:26Z

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

@@ -1529,6 +1560,7 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements

    const asgProps: CfnAutoScalingGroupProps = {
      autoScalingGroupName: this.physicalName,
+      availabilityZoneDistribution: props.availabilityZoneDistribution,


If we reduce nest, we can set property like this:

availabilityZoneDistribution: props.azCapacityDistributionStrategy ? { capacityDistributionStrategy: props.azCapacityDistributionStrategy } : undefined,

…vailability-zone-distribution

ren-yamanashi · 2024-12-19T00:23:52Z

Thank you for the contribution. I've commented on property design. If you have any thoughts on that please let me know.

@mazyu36

Thank you for review!
I have corrected the issue in the following commit, please confirm.

478f2ac

ren-yamanashi · 2024-12-19T00:29:02Z

@Mergifyio update

mergify · 2024-12-19T00:29:10Z

update

☑️ Nothing to do

#commits-behind > 0 [📌 update requirement]
-closed [📌 update requirement]
-conflict [📌 update requirement]
queue-position = -1 [📌 update requirement]

ren-yamanashi · 2024-12-19T00:55:50Z

@Mergifyio update

mergify · 2024-12-19T00:56:01Z

update

☑️ Nothing to do

#commits-behind > 0 [📌 update requirement]
-closed [📌 update requirement]
-conflict [📌 update requirement]
queue-position = -1 [📌 update requirement]

ren-yamanashi · 2024-12-19T01:00:47Z

I am getting the following error in Codecov / collect

Report creating failed: {“message”: “Token required because branch is protected”}

However, I do not have this branch protected and am not sure of the direct cause.

Also, CodeBuild is failing, but the changes I made do not seem to be the cause of the failure.

How can I resolve these issues?

mazyu36

Thanks! Some minor comments.
I'm not sure why Codecov failed.
Let's see if the issue persists after the changes are applied.

mazyu36 · 2024-12-19T09:33:49Z

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

+
+  /**
+   * The strategy for distributing instances across Availability Zones.
+   * @default BALANCED_BEST_EFFORT


Suggested change

* @default BALANCED_BEST_EFFORT

* @default CapacityDistributionStrategy.BALANCED_BEST_EFFORT

mazyu36 · 2024-12-19T09:34:13Z

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

+  /**
+   * If launches fail in an Availability Zone, Auto Scaling will attempt to launch in another healthy Availability Zone instead.
+   */
+  BALANCED_BEST_EFFORT= 'balanced-best-effort',


nit: space

Suggested change

BALANCED_BEST_EFFORT= 'balanced-best-effort',

BALANCED_BEST_EFFORT = 'balanced-best-effort',

ren-yamanashi · 2024-12-19T12:26:04Z

Thanks! Some minor comments. I'm not sure why Codecov failed. Let's see if the issue persists after the changes are applied.

@mazyu36

Thank you for comment!
I have corrected the issue, please confirm.
( I have included a link to the corrected commit in the thread)

Also, I fixed the default value of capacityDistributionStrategy was not applied correctly.
Please check this as well 🙏

bda5050

(Codecov seems to have fixed it too! Thank you 😆 )

mazyu36 · 2024-12-19T12:42:14Z

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

@@ -1530,6 +1550,9 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements

    const asgProps: CfnAutoScalingGroupProps = {
      autoScalingGroupName: this.physicalName,
+      availabilityZoneDistribution: props.azCapacityDistributionStrategy
+        ? { capacityDistributionStrategy: props.azCapacityDistributionStrategy }
+        : { capacityDistributionStrategy: CapacityDistributionStrategy.BALANCED_BEST_EFFORT },


Thank you for the fix.
But I think default value should be undefined.
The @default should describe the behavior at that time.

When the default value is explicitly set, it causes a breaking change.
This is because before and after this PR merged, the synth result changes when props.azCapacityDistributionStrategy is omitted.

@mazyu36

thank you!

I see, I understand.

So, I will also omit the description of default in JSDoc, is that correct?
(Because there was no mention of default values in the CloudFormation documentation...)
https://docs.aws.amazon.com/ja_jp/AWSCloudFormation/latest/UserGuide/aws-properties-autoscaling-autoscalinggroup-availabilityzonedistribution.html

/** * The strategy for distributing instances across Availability Zones. - * @default CapacityDistributionStrategy.BALANCED_BEST_EFFORT */ readonly azCapacityDistributionStrategy?: CapacityDistributionStrategy;

For optional properties, @default is necessary.

Also, it's common that default value behaviors are not documented in the CFn documentation.
In such cases, I often verify the default value by deploying without specifying the value.

When I tried deploying it without availabilityZoneDistribution property, the value became Balanced best effort, so I think this is the default value.

Thank you for the details.
I have corrected it with the following commit!

ea1e756

…vailability-zone-distribution

mazyu36

Thanks! LGTM

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

mergify · 2024-12-23T22:26:06Z

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).

mergify · 2024-12-23T22:30:19Z

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).

aws-cdk-automation · 2024-12-24T00:53:32Z

AWS CodeBuild CI Report

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

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

mergify · 2024-12-24T00:54:00Z

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).

github-actions · 2024-12-24T00:54:21Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

feat: support capacityDistributionStrategy

ec5bc2a

github-actions bot added the p2 label Nov 12, 2024

aws-cdk-automation requested a review from a team November 12, 2024 15:18

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Nov 12, 2024

ren-yamanashi changed the title ~~WIP: feat(aws-autoscaling): add availabilityZoneDistribution property to a AutoScalingGroup~~ feat(aws-autoscaling): add availabilityZoneDistribution property to a AutoScalingGroup Nov 12, 2024

feat: update README.md

bf7d04a

aws-cdk-automation previously requested changes Nov 12, 2024

View reviewed changes

feat: add integ test

0cb2e08

ren-yamanashi changed the title ~~feat(aws-autoscaling): add availabilityZoneDistribution property to a AutoScalingGroup~~ feat(autoscaling): add availabilityZoneDistribution property to a AutoScalingGroup Nov 12, 2024

ren-yamanashi added 4 commits November 13, 2024 00:48

feat: update README.md

41b597b

feat: update integ test

b51ccc3

Merge branch 'main' of https://github.com/aws/aws-cdk into feat-add-a…

39f4400

…vailability-zone-distribution

feat: add integ test

103c2b2

ren-yamanashi added 2 commits November 13, 2024 16:22

Merge branch 'main' of https://github.com/aws/aws-cdk into feat-add-a…

d84afbf

…vailability-zone-distribution

feat: add docstring

ce8ede9

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 13, 2024

ren-yamanashi changed the title ~~feat(autoscaling): add availabilityZoneDistribution property to a AutoScalingGroup~~ feat(autoscaling): add availabilityZoneDistribution property to an AutoScalingGroup Nov 23, 2024

Merge branch 'main' of https://github.com/aws/aws-cdk into feat-add-a…

d036a06

…vailability-zone-distribution

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

Monthly PR metrics report #32340

Open

Merge branch 'main' of https://github.com/aws/aws-cdk into feat-add-a…

6d7773a

…vailability-zone-distribution

mazyu36 reviewed Dec 18, 2024

View reviewed changes

ren-yamanashi added 2 commits December 19, 2024 08:58

Merge branch 'main' of https://github.com/aws/aws-cdk into feat-add-a…

8c8423b

…vailability-zone-distribution

feat: change props structure

478f2ac

mazyu36 reviewed Dec 19, 2024

View reviewed changes

ren-yamanashi added 3 commits December 19, 2024 21:04

feat: update jsDoc

385396e

feat: add space to enum property

a14ccad

feat: set default value

bda5050

mazyu36 reviewed Dec 19, 2024

View reviewed changes

ren-yamanashi added 2 commits December 19, 2024 22:16

Merge branch 'main' of https://github.com/aws/aws-cdk into feat-add-a…

fe42c32

…vailability-zone-distribution

feat: remove default value

ea1e756

mazyu36 approved these changes Dec 19, 2024

View reviewed changes

aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Dec 19, 2024

moelasmar reviewed Dec 23, 2024

View reviewed changes

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts Outdated Show resolved Hide resolved

resolve comment

0d535e9

moelasmar approved these changes Dec 23, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 23, 2024

Merge branch 'main' into feat-add-availability-zone-distribution

ba90575

mergify bot merged commit ecfce7c into aws:main Dec 24, 2024
17 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Dec 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(autoscaling): add `availabilityZoneDistribution` property to an AutoScalingGroup #32100

feat(autoscaling): add `availabilityZoneDistribution` property to an AutoScalingGroup #32100

ren-yamanashi commented Nov 12, 2024 •

edited

Loading

aws-cdk-automation left a comment

codecov bot commented Nov 24, 2024 •

edited

Loading

mazyu36 left a comment

mazyu36 Dec 18, 2024

mazyu36 Dec 18, 2024

ren-yamanashi commented Dec 19, 2024 •

edited

Loading

ren-yamanashi commented Dec 19, 2024

mergify bot commented Dec 19, 2024

ren-yamanashi commented Dec 19, 2024

mergify bot commented Dec 19, 2024

ren-yamanashi commented Dec 19, 2024 •

edited

Loading

mazyu36 left a comment

mazyu36 Dec 19, 2024

ren-yamanashi Dec 19, 2024

mazyu36 Dec 19, 2024

ren-yamanashi Dec 19, 2024

ren-yamanashi commented Dec 19, 2024 •

edited

Loading

mazyu36 Dec 19, 2024

ren-yamanashi Dec 19, 2024 •

edited

Loading

mazyu36 Dec 19, 2024

ren-yamanashi Dec 19, 2024

mazyu36 left a comment

mergify bot commented Dec 23, 2024

mergify bot commented Dec 23, 2024

aws-cdk-automation commented Dec 24, 2024

mergify bot commented Dec 24, 2024

github-actions bot commented Dec 24, 2024

	* @default BALANCED_BEST_EFFORT
	* @default CapacityDistributionStrategy.BALANCED_BEST_EFFORT

	BALANCED_BEST_EFFORT= 'balanced-best-effort',
	BALANCED_BEST_EFFORT = 'balanced-best-effort',

feat(autoscaling): add availabilityZoneDistribution property to an AutoScalingGroup #32100

feat(autoscaling): add availabilityZoneDistribution property to an AutoScalingGroup #32100

Conversation

ren-yamanashi commented Nov 12, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 24, 2024 • edited Loading

Codecov Report

mazyu36 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ren-yamanashi commented Dec 19, 2024 • edited Loading

ren-yamanashi commented Dec 19, 2024

mergify bot commented Dec 19, 2024

☑️ Nothing to do

ren-yamanashi commented Dec 19, 2024

mergify bot commented Dec 19, 2024

☑️ Nothing to do

ren-yamanashi commented Dec 19, 2024 • edited Loading

mazyu36 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

ren-yamanashi commented Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

ren-yamanashi Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mazyu36 left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 23, 2024

mergify bot commented Dec 23, 2024

aws-cdk-automation commented Dec 24, 2024

AWS CodeBuild CI Report

mergify bot commented Dec 24, 2024

github-actions bot commented Dec 24, 2024

feat(autoscaling): add `availabilityZoneDistribution` property to an AutoScalingGroup #32100

feat(autoscaling): add `availabilityZoneDistribution` property to an AutoScalingGroup #32100

ren-yamanashi commented Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 24, 2024 •

edited

Loading

ren-yamanashi commented Dec 19, 2024 •

edited

Loading

ren-yamanashi commented Dec 19, 2024 •

edited

Loading

ren-yamanashi commented Dec 19, 2024 •

edited

Loading

ren-yamanashi Dec 19, 2024 •

edited

Loading