-
Notifications
You must be signed in to change notification settings - Fork 36
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
upgrade eks module #29
Conversation
smalltown
commented
Apr 1, 2019
•
edited
Loading
edited
- upgrade eks module for supporting latest version
- delete worker-asg, worker-spot, worker-common module, creating eks-worker module by AWS launch template to support spot/on-demand EC2 instance type
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.
So many details ~~~
modules/aws/kube-worker/ami.tf
Outdated
@@ -0,0 +1,39 @@ | |||
locals { | |||
ami_owner = "595879546273" |
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.
How about use coreos_account
?
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.
no problem
modules/aws/eks/variables.tf
Outdated
default = "" | ||
} | ||
|
||
variable "cidr_access_eks_endpoint" { |
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.
How about to rename cidr_access_eks_https
for consistency with cidr_access_worker_ssh
?
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.
sure
modules/aws/eks/variables.tf
Outdated
default = "./terraform" | ||
} | ||
|
||
variable "write_kubeconfig" { |
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.
rename to kubeconfig_output_flag
?
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.
OK!
modules/aws/eks/variables.tf
Outdated
default = true | ||
} | ||
|
||
variable "write_aws_auth_config" { |
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.
rename to aws_auth_config_output_flag ?
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.
yes
@@ -0,0 +1,43 @@ | |||
locals { | |||
kubeconfig_name = "${var.kubeconfig_name == "" ? "${var.cluster_name}" : var.kubeconfig_name}" |
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.
Consider to give a default name ?
|
||
contexts: | ||
- context: | ||
cluster: ${kubeconfig_name} |
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.
Of course, it does not matter to use the same name, but I guess could consider to use ${cluster_name} here.