Skip to content

Conversation

@cyanidium
Copy link
Contributor

What this PR does / why we need it:
Only one app label should be set on kubernetes resources. fluxcd will refuse to install the chart with duplicate app labels. This removes a hardcoded app label and leaves the label based on the chart name in place.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #591

@cyanidium cyanidium requested a review from dougbtv as a code owner April 2, 2025 00:52
Common labels
*/}}
{{- define "whereabouts.labels" -}}
app: whereabouts
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that the app is defined below in selectorLabels, and the value is Chart.name, which is whereabouts-chart. I'm wondering if we should change this to just whereabouts instead. It’s just a cosmetic change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is unusual to have "chart" in the app name, yes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's tackle that in another pr

@coveralls
Copy link

Pull Request Test Coverage Report for Build 14209089615

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 51.575%

Totals Coverage Status
Change from base Build 13965567508: 0.0%
Covered Lines: 1965
Relevant Lines: 3810

💛 - Coveralls

Copy link
Collaborator

@mlguerrero12 mlguerrero12 left a comment

Choose a reason for hiding this comment

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

LGTM

@mlguerrero12 mlguerrero12 merged commit 3583797 into k8snetworkplumbingwg:master Apr 23, 2025
11 checks passed
@cyanidium cyanidium deleted the patch-1 branch April 23, 2025 21:03
@dhess
Copy link

dhess commented Jun 16, 2025

Thanks. Can we get a release for this fix? Version 0.9.0 of the Helm chart is not useable (with Flux, at least) without it.

@mlguerrero12
Copy link
Collaborator

@dhess, done, v0.9.1

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.

[BUG] Duplicate app label in helm template

4 participants