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(eks): EKS is now in Developer Preview #10518

Merged
merged 29 commits into from Sep 29, 2020
Merged

Conversation

iliapolo
Copy link
Contributor

@iliapolo iliapolo commented Sep 24, 2020

Graduate the module to Developer Preview.

This PR also includes some last minute API changes we feel make it a little more semantically clear.

In addition, the README was restructured to be more coherent. See rendered version.

Happy to break this down to multiple PR's if you find it hard to review.

BREAKING CHANGE: cluster.addChart renamed to cluster.addHelmChart

  • eks: NodegroupOptions.launchTemplate renamed to NodegroupOptions.launchTemplateSpec
  • eks: cluster.addAutoScalingGroup renamed to cluster.connectAutoScalingGroupCapacity
  • eks: cluster.addNodegroup renamed to cluster.addNoedgroupCapacity
  • eks: cluster.addCapacity renamed to cluster.addAutoScalingGroupCapacity
  • eks: CapacityOptions renamed to AutoScalingGroupCapacityOptions.

Resolves #10364


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

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 24, 2020
@iliapolo iliapolo marked this pull request as ready for review September 24, 2020 21:09
@iliapolo iliapolo added the pr/do-not-merge This PR should not be merged at this time. label Sep 24, 2020
version: eks.KubernetesVersion.V1_16,
endpointAccess: eks.EndpointAccess.PRIVATE // No access outside of your VPC.
});
```text
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still not a 100% sure of the value of this. Would love some input.

@@ -430,177 +458,104 @@ const cluster = eks.Cluster.fromClusterAttributes(this, 'MyCluster', {
> Instructions on how to build `layer.zip` can be found
> [here](https://github.com/aws-samples/aws-lambda-layer-kubectl/blob/master/cdk/README.md).

[aws-lambda-layer-kubectl]: https://github.com/aws-samples/aws-lambda-layer-kubectl
### VPC Support
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an entirely new section, would love your feedback on it.

to access the Kubernetes endpoint.

## Known Issues and Limitations
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are quite a few issues users should be aware of, even though there are explicitly mentioned in the relevant section, I also think it would be beneficial to have a clear simple section that emphasizes them.

@iliapolo iliapolo requested a review from a team September 29, 2020 09:38
eladb
eladb previously approved these changes Sep 29, 2020
packages/@aws-cdk/aws-eks/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-eks/README.md Outdated Show resolved Hide resolved
* [ARM64 Support](#arm64-support)
* [Kubectl Support](#kubectl-support)
* [VPC Support](#vpc-support)
* [Permissions and Access](#permissions-and-access)
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like it should be in a separate topic from "provisioning clusters"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, definitely some of it. Let me think of a better structure.

* [Fargate Profiles](#2-fargate-profiles)
* [Self Managed Auto Scaling Groups](#3-self-managed-auto-scaling-groups)
* [ARM64 Support](#arm64-support)
* [Kubectl Support](#kubectl-support)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this in "provisioning clusters"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it describes the kubectlLayer and kubectlEnvironment properties which are used to configure new clusters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like this should be under the "applying manifests" given this is where it's relevant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be a bit awkward since its relevant for helm charts as well. I think its ok to introduce this here as part of the cluster configuration, we already mentioned that this library supports applying manifests, so its not completely out of context. I'll add a link to this from the applying manifests section as well.

k?

* [API Reference](https://docs.aws.amazon.com/cdk/api/latest/docs/aws-eks-readme.html)
* [Architectural Overview](#architectural-overview)
* [Provisioning clusters](#provisioning-clusters)
* [Capacity Types](#capacity-types)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe hoist as a top level section

Copy link
Contributor Author

@iliapolo iliapolo Sep 29, 2020

Choose a reason for hiding this comment

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

Decided to get rid of this and just go ahead with detailing the various capacity types.

packages/@aws-cdk/aws-eks/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-eks/README.md Outdated Show resolved Hide resolved
@iliapolo iliapolo dismissed eladb’s stale review September 29, 2020 11:47

Review did not consider some comments

@iliapolo iliapolo requested a review from eladb September 29, 2020 11:47
@iliapolo iliapolo removed the pr/do-not-merge This PR should not be merged at this time. label Sep 29, 2020
@mergify
Copy link
Contributor

mergify bot commented Sep 29, 2020

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

@mergify
Copy link
Contributor

mergify bot commented Sep 29, 2020

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 81c319b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Sep 29, 2020

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

@mergify mergify bot merged commit b2ce3aa into master Sep 29, 2020
@mergify mergify bot deleted the epolon/10364-eks-dev-preview branch September 29, 2020 22:24
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[aws-eks] Graduate to Developer Preview
3 participants