-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: Remove app.kubernetes.io/managed-by
label
#591
Conversation
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.
app.kubernetes.io/managed-by
is a recommended label used by Helm Chart so we can avoid using this with Terraform replace this with terraform.io/module
. This will help both situations where customers either using Terraform or migrating to ArgoCD once deployed with Terraform.
@@ -5,8 +5,7 @@ resource "kubernetes_namespace" "spark" { | |||
} | |||
|
|||
labels = { | |||
job-type = "spark" | |||
"app.kubernetes.io/managed-by" = "terraform-aws-eks-blueprints" |
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.
We can keep this label and also add additional label
app.kubernetes.io/created-by = terraform-aws-eks-blueprints
@@ -2,10 +2,6 @@ resource "kubernetes_namespace_v1" "irsa" { | |||
count = var.create_kubernetes_namespace && var.kubernetes_namespace != "kube-system" ? 1 : 0 | |||
metadata { | |||
name = var.kubernetes_namespace | |||
|
|||
labels = { | |||
"app.kubernetes.io/managed-by" = "terraform-aws-eks-blueprints" |
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.
Can be renamed the key with app.kubernetes.io/created-by
. We can ignore managed-by
here as ArgoCD may takeover the management once we enable ArgoCD.
The same applies to all the Helm Charts add-ons and IRSA K8s resources
labels = merge( | ||
{ | ||
"app.kubernetes.io/managed-by" = "Terraform" | ||
"terraform.io/module" = "terraform-aws-eks-blueprints" |
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 keep this label
app.kubernetes.io/managed-by
foraws-auth
as its only managed by Terraformabut replace the value withterraform-aws-eks-blueprints
- We also need to add another label here
app.kubernetes.io/created-by = terraform-aws-eks-blueprints
This will ensure who created and who is currently managing the resource. managed-by
can be taken over by other sources like Helm when ArgoCD is enabled.
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.
Assuming there's no request and need by customers to have label such as "created-by" of eks-blueprints, I agree that we can remove it.
Later on, we can have a "labels" variable that can be passed instead of hard-coding this.
LGTM
What does this PR do?
app.kubernetes.io/managed-by
label from Terraform created resourcesMotivation
app.kubernetes.io/managed-by
tag with cluster-autoscaler (maybe others) #499More
pre-commit run -a
with this PRNote: Not all the PRs required examples and docs except a new pattern or add-on added.
For Moderators
Additional Notes