Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docs(autoscaling): improve documentation of default used for the desiredCapacity property #5208

Closed
wants to merge 1 commit into from
Closed

docs(autoscaling): improve documentation of default used for the desiredCapacity property #5208

wants to merge 1 commit into from
+3 −1

Conversation

shivlaks
Copy link
Contributor

@shivlaks shivlaks commented Nov 27, 2019 •
edited
Loading

Rationale: the default used for the desiredCapacity prop is dependent on other properties

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

@shivlaks shivlaks requested a review from rix0rrr as a code owner November 27, 2019 08:22
@mergify
Copy link
Contributor

mergify bot commented Nov 27, 2019

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 mergify bot added the contribution/core This is a PR that came from AWS. label Nov 27, 2019
@aws-cdk-automation
Copy link
Collaborator

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
rix0rrr requested changes Nov 27, 2019
View reviewed changes
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do this, but honestly I think the current behavior is wrong.

Wouldn't it be better to fix the behavior rather than to document it?

@shivlaks
Copy link
Contributor Author

We can do this, but honestly I think the current behavior is wrong.

Wouldn't it be better to fix the behavior rather than to document it?

Did you mean I should just adjust the behavior to match the current documentation? Only thing that made me pause was whether this would be considered a breaking change.

@kkelk kkelk mentioned this pull request Dec 2, 2019
Closed
2 tasks
@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

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

5 similar comments
@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

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
Copy link
Contributor

mergify bot commented Dec 16, 2019

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
Copy link
Contributor

mergify bot commented Dec 16, 2019

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
Copy link
Contributor

mergify bot commented Dec 16, 2019

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
Copy link
Contributor

mergify bot commented Dec 16, 2019

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 added a commit that referenced this pull request Dec 20, 2019
@rix0rrr
fix(autoscaling): every deployment resets capacity
da4ac95 
If `DesiredCapacity` is specified in the CloudFormation template, on every
deployment the capacity of the AutoScalingGroup is reset to that number,
even if the group had been scaled out at that point. The solution is to
leave DesiredCapacity empty, in which case it will remain untouched
during a deployment.

Previously, CDK would use some logic to always calculate a
DesiredCapacity for you, even if you left the `desiredCapacity` property
unset, leading to the undesirable behavior--which frankly represents
an availability risk.

Now, if you don't specify `desiredCapacity`, we won't set
`DesiredCapacity` either, avoiding the availability risk that we
introduced beforehand. In fact, if you *do* set `desiredCapacity`, we
will warn you that you probably shouldn't using a construct warning.

Fixes #5215, closes #5208.

BREAKING CHANGE: AutoScalingGroups without `desiredCapacity` are now
initially scaled to their minimum capacity (instead of their maximum
capaciety).
@rix0rrr rix0rrr mentioned this pull request Dec 20, 2019
Merged
@mergify mergify bot closed this in #5507 Dec 23, 2019
mergify bot added a commit that referenced this pull request Dec 23, 2019
@rix0rrr @mergify
fix(autoscaling): every deployment resets capacity (#5507)
0adf6c7 
* fix(autoscaling): every deployment resets capacity

If `DesiredCapacity` is specified in the CloudFormation template, on every
deployment the capacity of the AutoScalingGroup is reset to that number,
even if the group had been scaled out at that point. The solution is to
leave DesiredCapacity empty, in which case it will remain untouched
during a deployment.

Previously, CDK would use some logic to always calculate a
DesiredCapacity for you, even if you left the `desiredCapacity` property
unset, leading to the undesirable behavior--which frankly represents
an availability risk.

Now, if you don't specify `desiredCapacity`, we won't set
`DesiredCapacity` either, avoiding the availability risk that we
introduced beforehand. In fact, if you *do* set `desiredCapacity`, we
will warn you that you probably shouldn't using a construct warning.

Fixes #5215, closes #5208.

BREAKING CHANGE: AutoScalingGroups without `desiredCapacity` are now
initially scaled to their minimum capacity (instead of their maximum
capaciety).

* Add links

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
eladb pushed a commit that referenced this pull request Dec 24, 2019
@rix0rrr @mergify
fix(autoscaling): every deployment resets capacity (#5507)
641e12f 
* fix(autoscaling): every deployment resets capacity

If `DesiredCapacity` is specified in the CloudFormation template, on every
deployment the capacity of the AutoScalingGroup is reset to that number,
even if the group had been scaled out at that point. The solution is to
leave DesiredCapacity empty, in which case it will remain untouched
during a deployment.

Previously, CDK would use some logic to always calculate a
DesiredCapacity for you, even if you left the `desiredCapacity` property
unset, leading to the undesirable behavior--which frankly represents
an availability risk.

Now, if you don't specify `desiredCapacity`, we won't set
`DesiredCapacity` either, avoiding the availability risk that we
introduced beforehand. In fact, if you *do* set `desiredCapacity`, we
will warn you that you probably shouldn't using a construct warning.

Fixes #5215, closes #5208.

BREAKING CHANGE: AutoScalingGroups without `desiredCapacity` are now
initially scaled to their minimum capacity (instead of their maximum
capaciety).

* Add links

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@RomainMuller RomainMuller deleted the shivlaks/fix-asg-desiredcapacity-docs branch January 3, 2020 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rix0rrr rix0rrr rix0rrr requested changes

Assignees

@rix0rrr rix0rrr

Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shivlaks @aws-cdk-automation @rix0rrr