-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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: New Karpenter sub-module for easily enabling Karpenter on EKS #2303
Changes from all commits
2abc317
fe47884
3e1744e
af4aa27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,13 @@ provider "kubernetes" { | |
api_version = "client.authentication.k8s.io/v1beta1" | ||
command = "aws" | ||
# This requires the awscli to be installed locally where Terraform is executed | ||
args = ["eks", "get-token", "--cluster-name", module.eks.cluster_id] | ||
args = ["eks", "get-token", "--cluster-name", module.eks.cluster_name] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the reasoning for this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems more correct to pass in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see the PR changes for background and reasoning. more context can be found here hashicorp/terraform-provider-aws#27560 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for explaining. Will modify internally to match |
||
} | ||
} | ||
|
||
locals { | ||
name = "ex-${replace(basename(path.cwd), "_", "-")}" | ||
cluster_version = "1.22" | ||
cluster_version = "1.24" | ||
region = "eu-west-1" | ||
|
||
tags = { | ||
|
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.
in the future, can we do these sort of changes in a different PR?
a card that mentions karpenter, and all the sudden there is a major change of the cluster version, is not what anyone would expect
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 think this is just to the readme so probably not a big deal
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.
The PR description contains all the necessary details, it just seems some have not taken the time to read it 😉
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 sorry. I seem to not have notifications for PR, only releases and missed this PR when it was initially opened. And when I saw the release email, I clicked in the commit id rather than open the PR so never got to read the description.
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.
all good! I do try to put the relevant bits in the PR body with associated links as best as I can. its not always perfect though