-
Notifications
You must be signed in to change notification settings - Fork 104
Promtool-rules change configuration and add rule unit testing #1581
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
Conversation
| if err := yaml.Unmarshal(rawRules, &o.rules); err != nil { | ||
| return fmt.Errorf("failed to parse input rules: %v", err) | ||
| } | ||
| o.output, err = os.Create(o.outputBicep) |
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.
Why did this move out of complete? The point of this pattern is to have all necessary data read in and created here so testing past this step is easy.
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.
Oh, my point was, why create the output if the test could fail?
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.
Fair, but losing out on having options as useful input to execution is a shame.
2acc83d to
cacd555
Compare
72911bb to
a7ef1d7
Compare
tony-schndr
left a comment
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.
Overall this is great! Some improvements/nits for a follow up if you wish:
When testing I don't see any output on what tests were ran, it would be nice to see a format looking something like:
🟢 cluster-service/alert/InstancesDownV1 passed
❌ cluster-service/alert/InstancesDownV1 failed
I saw output only when it failed.
I also tried to generate the alerts in bicep, I updated the name InstancesDownV1 to InstancesDownV2 and I didn't see the update in the generated bicep module. Not sure if I was doing something wrong.
Maybe you're already working on it, but could we also get some documentation on how this is used?
|
Please rebase pull request. |
|
Great idea with the output. Let me take another look at this. I planned on documenting it once it's set. But I can add some docs in this PR |
|
@tony-schndr that is a bit complicated, since promtool does not provide this kind of output. I think it's not worth the effort, since the we are simply calling promtool. Give this test file my plan would be: for each |
|
@tony-schndr regarding the missing update, did you run |
Allows putting alerts into one folder. Propably want to change this to support multiple folders. Alternatively add a configuration struct to support multiple input folder configuration.
Call make alerts and ensure no uncommited files exists
f476d81 to
77511b9
Compare
ARO-15912
What
Adding call to promtool
Adding support for service owned alerts, where alerts folder and files are stored in service directory:
....
By putting alerts into service folders we can easily grant code ownership to these files.
These folders will be referenced by a configuration file in observability/alerts:
All files in rulesFolders must have tests
Files referenced in additionalRulesuntestedRules are exemptions and don't require a test
Why
Want to have strict testing for prometheus alerts
Need to support more than one alert file