Skip to content

fix(helm): inadvertent accumulation of podLabels using merge#16700

Merged
JStickler merged 7 commits intografana:mainfrom
schahal:fix/mutating-pod-labels
Mar 24, 2025
Merged

fix(helm): inadvertent accumulation of podLabels using merge#16700
JStickler merged 7 commits intografana:mainfrom
schahal:fix/mutating-pod-labels

Conversation

@schahal
Copy link
Contributor

@schahal schahal commented Mar 11, 2025

What this PR does / why we need it:

See this issue:

the labels under .Values.write.podLabels suddenly appear in all deployments and statefulsets because they are inadvertently leaked into .Values.loki.podLabels. The root cause of this problem can be traced back to a change in #16062, which incorrectly utilizes the merge function. The merge function modifies the source object[...]

Which issue(s) this PR fixes:

Fixes #16616 and #16617

Special notes for your reviewer:

I got the deepCopy suggestion from helm/helm#13308 (comment)

I tested chart locally before and after change and saw my podLabels as they should be

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@schahal schahal requested a review from a team as a code owner March 11, 2025 22:44
@schahal schahal mentioned this pull request Mar 11, 2025
3 tasks
Copy link

@balmeida-nokia balmeida-nokia left a comment

Choose a reason for hiding this comment

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

Suggesting an alternative way to deal with merge. Regardless, both ways are valid

@schahal schahal requested a review from balmeida-nokia March 12, 2025 14:14
@balmeida-nokia
Copy link

@schahal I'm not from loki, so I can't approve or reject. I just thought I could be useful. So I can't do anything productive about the review request, sorry.

@schahal schahal force-pushed the fix/mutating-pod-labels branch from d6e6fd6 to 5e414e4 Compare March 13, 2025 17:38
@schahal
Copy link
Contributor Author

schahal commented Mar 13, 2025

@poyzannur @bentonam @JStickler : I feel I followed Contributing.md and updated CHANGELOG just now as it'd suggested: is there anything else we should do to move this forward?

I tagged you as this should fix a bug that was introduced in #16062 and I'm stuck on the v1.26.0 release due to me needing specific labels on certain workloads (and prefer not to manage an out-of-band chart).

Thanks in advance!

@schahal schahal force-pushed the fix/mutating-pod-labels branch 3 times, most recently from bbfd7c0 to 22ad9d3 Compare March 18, 2025 20:39
@schahal schahal force-pushed the fix/mutating-pod-labels branch from 22ad9d3 to 0f6e6c0 Compare March 19, 2025 19:56
Signed-off-by: Satbir Chahal <satchahal@gmail.com>
@Jayclifford345
Copy link
Contributor

Hey @schahal, thanks for this I shall get this pushed through internally for review :)

@poyzannur
Copy link
Collaborator

Sorry to miss the ping @schahal
I remember the original PR, ah we missed that too.
Please rebase and resolve conflicts, otherwise it LGTM i'll be approving after.

@schahal
Copy link
Contributor Author

schahal commented Mar 24, 2025

Thanks @poyzannur ! All up-to-date!

@JStickler JStickler merged commit 717d97c into grafana:main Mar 24, 2025
70 checks passed
@schahal schahal deleted the fix/mutating-pod-labels branch March 24, 2025 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

component specific labels being added to other components in helm chart

5 participants