Skip to content
This repository has been archived by the owner on Jan 15, 2024. It is now read-only.

Add AlertRule type and CRUD method #91

Merged
merged 1 commit into from
Jul 6, 2022
Merged

Add AlertRule type and CRUD method #91

merged 1 commit into from
Jul 6, 2022

Conversation

stevenbrichards
Copy link
Contributor

Intending to add this for terraform support.

--

Based this on the example data examples in the API spec, so open to better test JSON / examples.

@CLAassistant
Copy link

CLAassistant commented Jun 28, 2022 •
edited

CLA assistant check
All committers have signed the CLA.

@stevenbrichards stevenbrichards mentioned this pull request Jun 28, 2022
Closed
@stevenbrichards stevenbrichards mentioned this pull request Jun 28, 2022
Closed
JohnnyQQQQ
JohnnyQQQQ suggested changes Jun 28, 2022
View reviewed changes
Copy link
Member

@JohnnyQQQQ JohnnyQQQQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Steven 👋 thanks a lot for your contribution, I left a few minor comments.

alerting_alert_rule.go Outdated Show resolved Hide resolved
JohnnyQQQQ
JohnnyQQQQ reviewed Jun 28, 2022
View reviewed changes
alerting_alert_rule.go Outdated Show resolved Hide resolved
@stevenbrichards
Copy link
Contributor Author

@JohnnyQQQQ ready for re-review

@JohnnyQQQQ
Copy link
Member

Thanks, the team member implementing this is currently on PTO and will take it from here as soon as he is back.

@stevenbrichards
Copy link
Contributor Author

Happy to implement / contribute more if pointed in the direction of need, just let me know :-) We're keen to codify our alarms in terraform.

@alexweav
Copy link
Contributor

Hi Steven, I'm actively working on this and the terraform integration for all alerting resources 👋

For Terraform to work efficiently and be consistent with Prometheus, we need the ability to query all rules in a group. There's an open PR in the main grafana repo adding an endpoint for this. It's awaiting review: grafana/grafana#51398

Once that PR goes in, we will then need a method here wrapping the new endpoint, then the terraform integration will be unblocked and good to go.

@stevenbrichards
Copy link
Contributor Author

stevenbrichards commented Jun 28, 2022 •
edited

Hey Alex, thanks for the update. While implementing this I noticed that POST /api/v1/provisioning/folder/{FolderUID}/rule-groups/{Group} is also missing from the client, but is in the API docs. I'd be glad to implement the GET/POST methods for this resource with your thumbs-up, in anticipation of the PR referenced being merged.

alexweav
alexweav suggested changes Jul 5, 2022
View reviewed changes
Copy link
Contributor

@alexweav alexweav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more points of feedback. These are all fairly nitpicky, this contribution is looking great!

No need to worry about the GET/PUT rule group endpoints as part of this PR quite yet. I'm actively working on those endpoints and will add them as soon as they're supported.

alerting_alert_rule.go Outdated Show resolved Hide resolved
alerting_alert_rule.go Outdated Show resolved Hide resolved
alerting_alert_rule.go Outdated Show resolved Hide resolved
alerting_alert_rule.go Outdated Show resolved Hide resolved
alerting_alert_rule.go Outdated Show resolved Hide resolved
alerting_alert_rule.go Outdated Show resolved Hide resolved
alerting_alert_rule.go Outdated Show resolved Hide resolved
alerting_alert_rule.go Outdated Show resolved Hide resolved
alerting_alert_rule_test.go
"github.com/gobs/pretty"
)

func TestAlertRules(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent tests. Thanks!

@stevenbrichards
Copy link
Contributor Author

@alexweav thanks, applied all your suggestions, I'll rebase and squash

@stevenbrichards
Copy link
Contributor Author

Good to go 👍

alexweav
alexweav approved these changes Jul 6, 2022
View reviewed changes
Copy link
Contributor

@alexweav alexweav left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly there - I think the tests need a couple touch-ups based on the rename suggestions + another pass with the auto-formatter, but otherwise this PR is good to go. Thanks!

@stevenbrichards @alexweav
Add AlertRule type and methods
61550ff 
Authored-by: Steven Richards <steven.richards@ookla.com>
Co-authored-by: Alexander Weaver <weaver.alex.d@gmail.com>
@stevenbrichards
Copy link
Contributor Author

Well that's embarrassing 😅 fixed and go fmt'd. Thanks @alexweav

@stevenbrichards
Copy link
Contributor Author

Tagged to re-review to merge

@alexweav alexweav merged commit e527c99 into grafana:master Jul 6, 2022
@stevenbrichards stevenbrichards deleted the add-alert-rule branch August 15, 2022 16:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JohnnyQQQQ JohnnyQQQQ JohnnyQQQQ approved these changes

@alexweav alexweav alexweav approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@stevenbrichards @CLAassistant @JohnnyQQQQ @alexweav