-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Conversation
version: eks.KubernetesVersion.V1_16, | ||
endpointAccess: eks.EndpointAccess.PRIVATE // No access outside of your VPC. | ||
}); | ||
```text |
There was a problem hiding this comment.
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.
packages/@aws-cdk/aws-eks/README.md
Outdated
@@ -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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
packages/@aws-cdk/aws-eks/README.md
Outdated
* [ARM64 Support](#arm64-support) | ||
* [Kubectl Support](#kubectl-support) | ||
* [VPC Support](#vpc-support) | ||
* [Permissions and Access](#permissions-and-access) |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
packages/@aws-cdk/aws-eks/README.md
Outdated
* [Fargate Profiles](#2-fargate-profiles) | ||
* [Self Managed Auto Scaling Groups](#3-self-managed-auto-scaling-groups) | ||
* [ARM64 Support](#arm64-support) | ||
* [Kubectl Support](#kubectl-support) |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
packages/@aws-cdk/aws-eks/README.md
Outdated
* [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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Review did not consider some comments
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). |
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
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 tocluster.addHelmChart
NodegroupOptions.launchTemplate
renamed toNodegroupOptions.launchTemplateSpec
cluster.addAutoScalingGroup
renamed tocluster.connectAutoScalingGroupCapacity
cluster.addNodegroup
renamed tocluster.addNoedgroupCapacity
cluster.addCapacity
renamed tocluster.addAutoScalingGroupCapacity
CapacityOptions
renamed toAutoScalingGroupCapacityOptions
.Resolves #10364
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license