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

Enable adding of additional Labels to speaker+controller #1797

Merged
merged 1 commit into from Mar 16, 2023

Conversation

DerFels
Copy link
Contributor

@DerFels DerFels commented Jan 24, 2023

Hi Metallb Team,
we have several labels that we like to add to our k8s resources. Some are purely for humans, while others are technically relevant (f.e. tracing, NetworkPolicies).
I noticed that your helm chart does not offer this functionality, so I wanted to help by adding a little code that enables users to specify additionaLabels in the values.yaml which will be picked up by the speaker+controller Daemonsets.

If there is anything missing or you disagree with the solution, I'm always open to feedback :)

Also: Thanks for providing a helm chart in the first place!

@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. This issue will be closed in 10 days unless you do one of the following:

  • respond or perform some activity on this PR
  • have one of these labels applied: hold

@DerFels
Copy link
Contributor Author

DerFels commented Feb 24, 2023

@fedepaol could you please take a look?
Thank you!

@@ -6,6 +6,11 @@ metadata:
labels:
{{- include "metallb.labels" . | nindent 4 }}
app.kubernetes.io/component: controller
{{- if .Values.controller.additionalLabels }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need this check. If I remember well, "with" and "range" do not generate output if the array is empty.

Copy link
Member

Choose a reason for hiding this comment

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

right!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @mlguerrero12 , I wasn't aware of this!

@mlguerrero12
Copy link
Contributor

/lgtm

@fedepaol
Copy link
Member

@DerFels please run inv helmdocs to regenerate the helm docs (ci is annoying but prevents errors :-) )

@DerFels
Copy link
Contributor Author

DerFels commented Mar 15, 2023

@DerFels please run inv helmdocs to regenerate the helm docs (ci is annoying but prevents errors :-) )

Done :)

@fedepaol fedepaol merged commit 73b8eb9 into metallb:main Mar 16, 2023
23 of 24 checks passed
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.

None yet

3 participants