-
Notifications
You must be signed in to change notification settings - Fork 132
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
enhancement(prometheus): support prometheus2 .yml rule file format #333
Conversation
Docs Build 📝Thank you for contribution!✨ This PR has been merged and the docs are now incorporated into |
Signed-off-by: dpavle <[email protected]>
Co-authored-by: Ben Kochie <[email protected]> Signed-off-by: Pavle <[email protected]>
Please also update the updated docs. |
Signed-off-by: dpavle <[email protected]>
There are some other references to |
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.
If we want to support both .rules
and .yml
as proposed by this PR then both should be listed in the docs and argument specs.
But if we want to drop .rules
then that should be done clearly with a warning in the preflight.
Also .yaml
should be supported since both.yml
and .yaml
are valid yaml file extensions.
I'm for removing the old As for the warning in the preflight, sure, that could be done. I'm just not sure how exactly, do we fail if any And yes, |
That's why I suggested a error in the preflight, but we could also make that a warning for now and then drop |
Signed-off-by: dpavle <[email protected]>
…les extension Signed-off-by: dpavle <[email protected]>
Signed-off-by: dpavle <[email protected]>
A warning works. If there are any YAML formatted files with the .rules extension there will be a warning message in the preflight and the rule validation will pass normally. On the other hand, if there are any rules in the old .rules format, the warning message will show in the preflight and promtool check validation will catch them and fail in a later step. |
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.
LGTM
@dpavle any chance you could refactor this so that we can get it merged? |
sure, I'll take a look at it |
@SuperQ any outstanding issues or are we good to go? |
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.
Yup, LGTM
prometheus-community/ansible#333 Signed-off-by: gardar <[email protected]>
No description provided.