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

Helm: add commonLabels (#9067) #10385

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Helm: add commonLabels (#9067) #10385

wants to merge 3 commits into from

Conversation

ftrigari
Copy link

@ftrigari ftrigari commented Jan 9, 2025

What this PR does
Every object created by the chart could have custom labels.

Which issue(s) this PR fixes or relates to
Fixes #9067

--- EDIT ---

I made a mess with my branch. I erased all and created a new commit.
This is the old PR #9742

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@ftrigari ftrigari requested a review from a team as a code owner January 9, 2025 13:35
@ftrigari ftrigari mentioned this pull request Jan 9, 2025
4 tasks
Copy link
Contributor

@NickAnge NickAnge left a comment

Choose a reason for hiding this comment

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

Thanks @ftrigari for the change. Could you please update CHANGELOG, with the addition ? Thanks in advance

@ftrigari
Copy link
Author

Hi @NickAnge,
I updated the changelog file.
Thank you

Signed-off-by: fabio trigari <trigarifabio@gmail.com>
Signed-off-by: fabio trigari <trigarifabio@gmail.com>
Signed-off-by: fabio trigari <trigarifabio@gmail.com>
@narqo
Copy link
Contributor

narqo commented Jan 13, 2025

Could you add a unit-test for this change? I don't think it's worth having a separate test, so maybe only add a simple case to the existing test-oss-values.yaml.

After that, you'll need to run make build-helm-tests in the project's root to regenerate the assets for this test. With that it'll be easier to track future changes around the values, to make sure we don't break it.

Comment on lines +80 to +82
# -- Common labels for all object directly managed this chart.
# scope: *
commonLabels: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what is the use-case for the feature, but I should point out that — in the current implementation — the global.commonLabels seem to only add labels to the top-level resources (e.g. deployments, statefulsets, configmaps, etc). It doesn't add labels to the resources, controlled by those top-levels, e.g. pods, that a deployment creates. This may be fine.

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.

Add possibility do add common labels in helm chart
4 participants