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

Ensure order of recording rule registration in PrometheusRule #11

Merged

Conversation

fossedihelm
Copy link
Contributor

It could happen that there are several
prometheus rules with the same name but different
expression.
Since the range in a for loop does not provide
always the same order, it could happen the
resulting prometheus rule list to be different.

@machadovilaca
Copy link
Owner

thank you for the PR @fossedihelm
can you improve the unit test -

It("should sort the recording rules of alerts by name ('Record')", func() {

to have 2 recording rules with the same name, and make sure they are sorted by the expression?

@machadovilaca
Copy link
Owner

I think we should also change the PR title to reflect the changes
wdyt about "Ensure order of recording rule registration in PrometheusRule"?

@fossedihelm fossedihelm changed the title Allow prometheusrules with same name and different expression Ensure order of recording rule registration in PrometheusRule Apr 16, 2024
It could happen that there are several
prometheus rules with the same name but different
expression.
Since the `range` in a for loop does not provide
always the same order, it could happen the
resulting prometheus rule list to be different.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@machadovilaca
Copy link
Owner

This unit test definitely needs to be refactored
I need to take a look at it later

Thank you for the fix 🙌

@machadovilaca machadovilaca merged commit 4d1d8e0 into machadovilaca:main Apr 16, 2024
1 check passed
@fossedihelm
Copy link
Contributor Author

This unit test definitely needs to be refactored I need to take a look at it later

Thank you for the fix 🙌

Agree! Honestly, I didn't dwell much on refactoring it, due to the hotfix :)
Thank you for your fast interaction

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

2 participants