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.operator.probes] Add namespace label with the namespace of the probe object #1058

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

Conversation

ToonTijtgat2
Copy link

@ToonTijtgat2 ToonTijtgat2 commented Jun 17, 2024

PR Description

Add namespace label with the namespace of the probe object in the static config to align with prometheus setup.
Prometheus automatically adds the namespace label, and since this is standard practice and we rely on this label for some dashboards and rules, we'd like to align the prometheus setup and alloy setup.

Which issue(s) this PR fixes

Fixes #1049

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@ToonTijtgat2
Copy link
Author

@thampiotr Thanks for running the tests, I adapted the code a little according to the results. Can you try again?
Thanks

@ToonTijtgat2
Copy link
Author

@thampiotr Thanks for running the tests again, for the failing results on this one, I'm not sure what the issues are or how to solve them.

@ToonTijtgat2
Copy link
Author

@thampiotr Noticed that the probe config tests are not successful, so added the expectation of a label with name namespace and value default.

@ToonTijtgat2
Copy link
Author

@thampiotr I changed the labelset from an array of sets to one object.

@ToonTijtgat2
Copy link
Author

@thampiotr, Is there something else for the moment I can do? (not sure how the full PR flow works)
Thanks for checking

@ToonTijtgat2
Copy link
Author

@thampiotr Adapted the test to have one list instead of a list in a list

@thampiotr
Copy link
Contributor

@ToonTijtgat2 you should be able to run most of the tests locally with make test, there are also some contribution docs in the repository you may find helpful. I intend to take a closer review once we have tests passing.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ ToonTijtgat2
❌ Tijtgat Toon


Tijtgat Toon seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ToonTijtgat2
Copy link
Author

@thampiotr it seems that all tests passed now, what do we have to do now to complete this PR?

I saw that by accident I did one of the commits with an unknown user. I don't know if this is a problem about the cla?
I updated the fork a few moments ago to fetch the latest changes.
But according to me this PR seems to look good now.

Thanks for checking.

FYI, I try to run the tests locally but for a reason I get the following all the time.
image

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.

[prometheus.operator.probes] add namespace label
3 participants