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

Add recommended Kubernetes labels #217

Merged
merged 14 commits into from
Oct 15, 2021

Conversation

lindhe
Copy link
Contributor

@lindhe lindhe commented Oct 7, 2021

This change implements the recommended Kubernetes labels where applicable, according to Helm's recommendations.

Resolves #216

Status

Label Status Value
app.kubernetes.io/name Done {{ template "splunk-otel-collector.name" . }}
helm.sh/chart Done {{ .Chart.Name }}
app.kubernetes.io/managed-by Done {{ .Release.Service }}
app.kubernetes.io/instance Done {{ .Release.Name }}
app.kubernetes.io/version Done {{ .Chart.AppVersion | quote }}
app.kubernetes.io/component Done otel-k8s-cluster-receiver or otel-collector
app.kubernetes.io/part-of Omitted n/a
app.kubernetes.io/created-by Omitted n/a

TODO

  • Design decision on anything that's not "Done" or "Omitted"
  • Move common labels to _helpers.tpl
  • Sort the labels. Alphabetically or in the same order as in the Helm documentation?
  • CHANGELOG
  • make render
  • Final review from maintainers

@lindhe lindhe requested review from a team as code owners October 7, 2021 12:32
@dmitryax
Copy link
Contributor

dmitryax commented Oct 7, 2021

@lindhe please run make render and add a line to CHANGELOG

@lindhe
Copy link
Contributor Author

lindhe commented Oct 8, 2021

Again, note that this PR only applies the labels where I think it matches the existing labels. Do we want to look in more details for places where we want to introduce the remaining labels as well, or do we take that as a separate PR?

@dmitryax
Copy link
Contributor

dmitryax commented Oct 8, 2021

@lindhe it would be better if we can add other labels all at once in this PR. I think we should also add helm.sh/chart, right?

And I would move them all to a template helper in another PR. Not asking you to do that, just making a note for myself.

@dmitryax
Copy link
Contributor

dmitryax commented Oct 8, 2021

And sorry, I pushed something else, and it needs another make render now :)

@lindhe
Copy link
Contributor Author

lindhe commented Oct 11, 2021

it would be better if we can add other labels all at once in this PR. I think we should also add helm.sh/chart, right?

I agree! It will be somewhat tedious to go through all components and think about which new labels make sense, but I think it will be worth it. Great that you remembered helm.sh/chart! I forgot that one, since it's in Helm's list of common labels but not Kubernetes'. I'll add it to PR and issue description.

And I would move them all to a template helper in another PR. Not asking you to do that, just making a note for myself.

Moving them to a template helper sounds like a good idea. There's two reasons I didn't do that already:

  1. The component label existed in some objects but not all. If that distinction is important, then it's hard to keep it centrally in _helpers.tpl.
  2. I'm not sure that all recommended Kubernetes labels makes sense on every object. Like for example, does a configMap object have a version?

And sorry, I pushed something else, and it needs another make render now :)

No worries! It's great that the new releases come so frequently, just keep that going and we'll fix conflicts here later! :) But I'm probably going to wait with another make render until we're happy with the state of this PR (i.e. all new labels are added), so we don't have to do it over and over.

Another question: Do we want app.kubernetes.io/created-by at all? I think it's hard to define...

@lindhe lindhe force-pushed the lindhe/recommended-labels branch from ad02a6c to 7324c8f Compare October 12, 2021 08:22
@lindhe
Copy link
Contributor Author

lindhe commented Oct 12, 2021

I removed the CHANGELOG commit and make render commit to avoid having to resolve merge conflicts all the time when working.

TODO: Add those back in later.

@lindhe lindhe force-pushed the lindhe/recommended-labels branch from 2458e75 to 35eb883 Compare October 12, 2021 08:48
@dmitryax
Copy link
Contributor

dmitryax commented Oct 13, 2021

  1. The component label existed in some objects but not all. If that distinction is important, then it's hard to keep it centrally in _helpers.tpl.

I would move only common labels to the template helper, while keeping the component labels on their places.

  1. I'm not sure that all recommended Kubernetes labels makes sense on every object. Like for example, does a configMap object have a version?

I think we should add all the recommended labels uniformly across all k8s objects. I don't see any issue with configMap having app version label

Another question: Do we want app.kubernetes.io/created-by at all? I think it's hard to define...

I agree, let's omit it.

As recommended by Helm:

> helm.sh/chart:
> This should be the chart name and version: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}.

https://helm.sh/docs/chart_best_practices/labels/#standard-labels
@lindhe
Copy link
Contributor Author

lindhe commented Oct 14, 2021

I had missed that helm.sh/chart should be the chart name and version, so I'm fixing that! At first, I had only set it to .Chart.Name.

* app.kubernetes.io/name
* app.kubernetes.io/managed-by
* app.kubernetes.io/instance
* app.kubernetes.io/version
* helm.sh/chart
@lindhe
Copy link
Contributor Author

lindhe commented Oct 14, 2021

There. Consolidating them into a shared definition in _helpers.tpl massively improved the readability of this PR.

Please review the new definitions and decide if it looks good.

Is the label sorted in a good order? I kept it the same as in https://helm.sh/docs/chart_best_practices/labels/#standard-labels

@dmitryax
Copy link
Contributor

Is the label sorted in a good order? I kept it the same as in https://helm.sh/docs/chart_best_practices/labels/#standard-labels

Yes, I think this order is good.

Copy link
Contributor

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Look good to me. Thanks the contribution!

@dmitryax dmitryax merged commit b096518 into signalfx:main Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The chart is missing recommended Kubernetes labels
2 participants