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

[prometheus-blackbox-exporter] use well known label as namespace label selector #3217

Closed
wants to merge 7 commits into from

Conversation

PG2000
Copy link

@PG2000 PG2000 commented Apr 11, 2023

What this PR does / why we need it

Kubernetes uses a well known label for namespaces. (kubernetes.io/metadata.name=)
Would be great if the network policy can match this label then.

Which issue this PR fixes

none

Special notes for your reviewer

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

@PG2000 PG2000 changed the title feat: use well known label for namespace label [prometheus-blackbox-exporter] use well known label as namespace label selector Apr 11, 2023
Copy link
Member

@monotek monotek left a comment

Choose a reason for hiding this comment

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

i don't see why this should be the default, breaking the chart for everyone not having this label.

@PG2000
Copy link
Author

PG2000 commented Apr 11, 2023

i don't see why this should be the default, breaking the chart for everyone not having this label.

Sorry i missed to give some context.
I did this based on https://kubernetes.io/docs/reference/labels-annotations-taints/#kubernetes-io-metadata-name

@QuentinBisson
Copy link
Member

@monotek this label is set by default on all namespaces since at least kubernetes 1.22 (c.f. https://v1-22.docs.kubernetes.io/docs/reference/labels-annotations-taints/#kubernetes-io-metadata-name)

@PG2000 I would suggest you add a kubernetes version flag so the chart uses the old way prior to 1.22 and the new way after 1.22.

Would that work for the both of you?

@monotek
Copy link
Member

monotek commented Apr 13, 2023

I would just make the labels generical configurable, using the old labels as default.

Signed-off-by: pg2000 <10741029+PG2000@users.noreply.github.com>
Signed-off-by: pg2000 <10741029+PG2000@users.noreply.github.com>
Signed-off-by: pg2000 <10741029+PG2000@users.noreply.github.com>
@PG2000
Copy link
Author

PG2000 commented Apr 14, 2023

I would just make the labels generical configurable, using the old labels as default.

I did the mentioned label selector configurable.

Just from my point of view:
The new label would makes this work out of the box for everyone with a version > 1.21

I didn't get why this should be backwards compatible for end of life k8s versions.

From my point of view:
For this you have some semantic versioning of the helm chart and more effort to maintain this with all the stuff just to be backwards compatible for end of life k8s versions.

Signed-off-by: pg2000 <10741029+PG2000@users.noreply.github.com>
Signed-off-by: pg2000 <10741029+PG2000@users.noreply.github.com>
@PG2000 PG2000 requested a review from monotek April 23, 2023 17:49
@QuentinBisson
Copy link
Member

@PG2000 can you bump the version?

@stale
Copy link

stale bot commented Jun 23, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale
Copy link

stale bot commented Aug 7, 2023

This issue is being automatically closed due to inactivity.

@stale stale bot closed this Aug 7, 2023
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.

None yet

3 participants