-
Notifications
You must be signed in to change notification settings - Fork 111
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
General Upgrades and Housekeeping for Terraform bootstrap #193
Conversation
this PR fixes #166 |
bootstrap/terraform/main.tf
Outdated
name = "crossplane-blueprints" | ||
region = "us-east-1" |
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.
What is the reason for removing the vars here?
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've seen the EKS Blueprints adopting this pattern of not having the variables file, I think it makes it easier for the user:
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.
keep the vars here as well, we need them to be able to pass and override on command 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.
done
bootstrap/terraform/main.tf
Outdated
cluster_version = "1.30" | ||
capacity_type = "SPOT" |
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.
same question, what is the reason for removing the vars?
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 same here:
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.
same here, keep the vars
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.
done
bootstrap/terraform/main.tf
Outdated
values = [ | ||
templatefile("${path.module}/argocd-values.yaml", { | ||
templatefile("${path.module}/values/control-plane-eks-argocd-stack.yaml", { |
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 is nice to organize under values/ folder but why prefix with control-plane-eks-
?
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 is no specific reason.. I thought it would be clearer, but I can remove 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.
removed
bootstrap/terraform/main.tf
Outdated
chart = "crossplane" | ||
chart_version = "1.16.0" | ||
repository = "https://charts.crossplane.io/stable/" | ||
values = [file("${path.module}/values/control-plane-eks-crossplane-stack.yaml")] |
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 touch! Love it! Much more readable!
bootstrap/terraform/main.tf
Outdated
@@ -210,8 +207,8 @@ locals { | |||
crossplane_namespace = "crossplane-system" | |||
|
|||
upjet_aws_provider = { | |||
enable = var.enable_upjet_aws_provider # defaults to true | |||
version = "v1.4.0" | |||
enable = true |
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.
keep the variable
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.
added back
bootstrap/terraform/main.tf
Outdated
@@ -233,15 +230,15 @@ locals { | |||
} | |||
|
|||
aws_provider = { | |||
enable = var.enable_aws_provider # defaults to false | |||
enable = false |
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.
keep the variable
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.
added back
bootstrap/terraform/main.tf
Outdated
version = "v0.48.0" | ||
name = "aws-provider" | ||
runtime_config = "aws-runtime-config" | ||
provider_config_name = "aws-provider-config" #this is the providerConfigName used in all the examples in this repo | ||
} | ||
|
||
kubernetes_provider = { | ||
enable = var.enable_kubernetes_provider # defaults to true | ||
enable = true |
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.
keep the variable
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.
added back
bootstrap/terraform/main.tf
Outdated
@@ -251,7 +248,7 @@ locals { | |||
} | |||
|
|||
helm_provider = { | |||
enable = var.enable_helm_provider # defaults to true | |||
enable = true |
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.
keep the variable
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.
added back
@candonov adjusted it with your recommendations |
bootstrap/terraform/main.tf
Outdated
values = [ | ||
templatefile("${path.module}/argocd-values.yaml", { | ||
templatefile("${path.module}/values/argocd-stack.yaml", { |
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.
Let's have the file name match the add-on name, argocd.yaml
bootstrap/terraform/main.tf
Outdated
values = [file("${path.module}/kube-prometheus-stack-values.yaml")] | ||
wait = true | ||
timeout = "600" | ||
values = [file("${path.module}/values/prometheus-stack.yaml")] |
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.
kube-prometheus-stack
bootstrap/terraform/main.tf
Outdated
chart = "crossplane" | ||
chart_version = "1.16.0" | ||
repository = "https://charts.crossplane.io/stable/" | ||
values = [file("${path.module}/values/crossplane-stack.yaml")] |
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.
crossplane.yaml
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
What does this PR do?
gavinbunney/kubectl
.Motivation
The motivation behind this pull request is to enhance maintainability, and ensure that our Blueprints adhere to the latest best practices.
More
Note:
For Moderators
Additional Notes
N/A