-
Notifications
You must be signed in to change notification settings - Fork 986
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: auto-detect clusterendpoint for eks #3363
Conversation
✅ Deploy Preview for karpenter-docs-prod canceled.
|
@ellistarn @jonathan-innis Anything else I should add? |
@jonathan-innis I just moved the resolving out of the launchtemplate, the endpoint address is then stored in the settings for the rest of the controllers lifetime. |
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.
LGTM, I think we can just add the clusterEndpoint into the LaunchTemplateProvider constructor
Co-authored-by: Jonathan Innis <jonathan.innis.ji@gmail.com>
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.
Nice PR! This will be great to reduce the burden of upfront configuration! Just a few suggestions and docs update requests.
Oh, I just saw that auto-formatting changed the layout of the markdown quite significantly, I'll revert the change and update only the single line. |
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.
Few comments on the docs changes. Looking good!
website/content/en/preview/getting-started/getting-started-with-eksctl/cloudformation.yaml
Outdated
Show resolved
Hide resolved
Can you also add a unit test (with mocking on the EKS API) to ensure that we are able to resolve this value if it isn't set? If you need any help figuring out where to go for this, you can ping me on Slack or on GH, happy to help. |
@jonathan-innis sure thing, please have a look, if I found the right spots 😄 |
Co-authored-by: Brandon Wagner <bmwagner10@gmail.com>
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.
LGTM 🚀
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.
LGTM
Fixes #3192
Description
Adds auto-detection of the clusterEndpoint if it's eks, based on the provided clusterName.
How was this change tested?
Does this change impact docs?
Release Note
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.