-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
hcloud discovery: Add testcase for key only labels #9028
hcloud discovery: Add testcase for key only labels #9028
Conversation
41ddaa4
to
064d82b
Compare
Signed-off-by: Lukas Kämmerling <lukas.kaemmerling@hetzner-cloud.de>
064d82b
to
9823fef
Compare
Signed-off-by: Lukas Kämmerling <lukas.kaemmerling@hetzner-cloud.de>
This should be a new label, which is there for all the labels. |
(the new label should have labelpresent instead of label), see kubernetes_sd doc. |
Signed-off-by: Lukas Kämmerling <lukas.kaemmerling@hetzner-cloud.de>
@roidelapluie i changed it, thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It should be for every label.
@roidelapluie changed :) |
591a053
to
90ee71c
Compare
discovery/hetzner/hcloud.go
Outdated
presentLabel := model.LabelName(hetznerLabelHcloudLabelPresent + strutil.SanitizeLabelName(labelKey)) | ||
labels[presentLabel] = model.LabelValue("true") | ||
if labelValue == "" { | ||
label := model.LabelName(hetznerLabelHcloudLabel + strutil.SanitizeLabelName(labelKey)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this if should go away. We should have the label for every hcloud label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense, why should the label be there if prometheus skips it at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh it is the normal label :)
Any way works, I just prefer less if
branches.
I guess the tests should fail and you will need to rebase to fix the DCO. Thanks! |
Signed-off-by: Lukas Kämmerling <lukas.kaemmerling@hetzner-cloud.de>
9ba57fb
to
0dc2518
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last nits: Can you remove the if branch, and add another (filled) label in the tests?
@roidelapluie i added this case :) |
510f647
to
ea197a2
Compare
Signed-off-by: Lukas Kämmerling <lukas.kaemmerling@hetzner-cloud.de>
ea197a2
to
542975f
Compare
Thanks! |
This adds a test case for the hcloud discovery with key only labels.
Key Only Labels should already be possible as they are returned from the hcloud API
{"key":""}
Fixes #9023