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.

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

feat(aws-eks): add construct library for EKS #1655

Merged
merged 12 commits into from
Feb 8, 2019
Merged

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Feb 1, 2019

Construct library to set up an EKS cluster and add nodes to it.

Generalizes adding AutoScalingGroup capacity and make it the same
between both ECS and EKS clusters. Fixes naming inconsistencies
among properties of AutoScalingGroup, and between EC2 AutoScaling and
Application AutoScaling.

This PR takes #991 but reimplements the API in the style of the ECS library.

BREAKING CHANGE: For AutoScalingGroup, renamed minSize =>
minCapacity, maxSize => maxCapacity, for consistency with
desiredCapacity and also Application AutoScaling.

For ECS's addDefaultAutoScalingGroupCapacity(), instanceCount =>
desiredCapacity and the function now takes an ID (pass
"DefaultAutoScalingGroup" to avoid interruption to your deployments).

Again, unit tests are lacking for now, would like feedback on the shape of the API first.


Pull Request Checklist

  • Testing
    • Unit test added
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

Original work done by @IPyandy
Generalize and conformize adding AutoScalingGroup capacity to both ECS
and EKS clusters, as well as naming consistency among variables and with
Application AutoScaling.

BREAKING CHANGE: For `AutoScalingGroup`, renamed `minSize` =>
`minCapacity`, `maxSize` => `maxCapacity`, for consistency with
`desiredCapacity` and also Application AutoScaling.

For ECS's `addDefaultAutoScalingGroupCapacity()`, `instanceCount` =>
`desiredCapacity` and the function now takes an ID (pass
`"DefaultAutoScalingGroup"` to avoid interruption to your deployments).
@rix0rrr rix0rrr requested review from SoManyHs and a team as code owners February 1, 2019 16:29
Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

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

That looks fair to me... @SoManyHs gets the last word 😄

packages/@aws-cdk/aws-eks/lib/ami.ts Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/lib/cluster.ts Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/lib/cluster.ts Show resolved Hide resolved
packages/@aws-cdk/aws-eks/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-eks/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-eks/lib/cluster.ts Show resolved Hide resolved
packages/@aws-cdk/aws-eks/lib/cluster.ts Show resolved Hide resolved
*
* The nodes will automatically be configured with the right VPC and AMI
* for the instance type and Kubernetes version.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also be possible to do via ctor (thinking of deCDK)

*
* Prefer to use `addWorkerNodes` if possible.
*/
public addAutoScalingGroup(autoScalingGroup: autoscaling.AutoScalingGroup, options: AddAutoScalingGroupOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is so awesome!

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a link ref to what this is doing under the hood

packages/@aws-cdk/aws-eks/lib/cluster.ts Show resolved Hide resolved
@rix0rrr rix0rrr merged commit 22fc8b9 into master Feb 8, 2019
@rix0rrr rix0rrr deleted the huijbers/eks-cp branch February 8, 2019 17:51
@fulghum fulghum added the effort/medium Medium work item – several days of effort label Feb 11, 2019
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants