Skip to content
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

Managed-by should only be set by helm or similar at install time #294

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

bleggett
Copy link
Contributor

@bleggett bleggett commented Oct 29, 2024

  1. app.kubernetes.io/managed-by: Helm is automatically applied to resources installed by Helm, and does not need to be (read: should not be) manually templated. The label is an install-time label that should be applied by the tool actually installing the resources into the cluster.

This matters because helm template should (and by default will) produce YAML without this label, as it is unknown at template-time what will "manage" the resource.

  1. Also fix the makefile so it doesn't try to download the wrong Helm bin on linux/arm64.

(@jmazzitelli added the fixes line below)
fixes: istio/istio#53698

Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
@jmazzitelli
Copy link
Contributor

jmazzitelli commented Oct 30, 2024

This looks OK. Before we review/merge, can you create a github issue and link this PR to it so it can be tracked?

Please see the contributor's guide for details: https://github.com/kiali/kiali/blob/master/CONTRIBUTING.md#making-a-change

UPDATE: I see this was a result of an upstream Istio issue... let's just link the upstream Istio issue with this PR

(By link I mean in the description, just add fixes: https://github.com/istio/istio/issues/53698 in the text)

UPDATE 2: I added that to the issue description

Copy link
Contributor

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

to test:

  1. build the helm charts: make build-helm-charts
  2. see that helm template output no longer adds the managed-by label:
helm template -n istio-system kiali-server _output/charts/kiali-server-*.tgz | grep managed-by
  1. Install using helm install and see that it does include the managed-by label:
helm install --namespace istio-system kiali-server _output/charts/kiali-server-*.tgz
$ kubectl get -n istio-system clusterroles,deployment,cm,sa -l app.kubernetes.io/name=kiali -oyaml | grep Helm
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/managed-by: Helm
  1. Do the same with the operator chart
helm template -n default kiali-operator _output/charts/kiali-operator-*.tgz | grep managed-by
helm install --namespace default kiali-operator _output/charts/kiali-operator-*.tgz
$ kubectl get -n default clusterroles,deployment,sa -l app.kubernetes.io/name=kiali-operator -oyaml | grep Helm
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/managed-by: Helm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

samples include "managed-by: Helm" label
2 participants