Panel to target migration #37
Conversation
| if !panelHasQueries(p) { | ||
| // Don't lint certain types of panels. | ||
| return ResultSuccess | ||
| } |
There was a problem hiding this comment.
I'm not convinced this check is necessary anymore, since this will only be run for panels which have targets (so it would naturally exclude rows, text, and other non dataviz panels) and we're already checking that the panel is of the correct "type", namely that it uses a prometheus DS.
| default: | ||
| t.Errorf("No concrete target required matcher rule for '%s", matcher) | ||
| return | ||
| } |
There was a problem hiding this comment.
(Super minor nit, feel free to ignore) To avoid this I'd be tempter to pass the linter in the testcase structs below, or even stick an inner loop inside the test (will give an example).
There was a problem hiding this comment.
Yeah, I agonized over that a bit. Ultimately the bigger problem is getting the error messages to match up with the provided name, so I'll leave it as-is for now.
I'm actually planning some additional changes that will allow specific exceptions to the names of these (sometimes job is actually named cluster, and sometimes instance is actually called node), so I'll refactor the tests at that point.
Thanks for the review!
| }, | ||
| } | ||
|
|
||
| testRule(t, linter, dashboard, tc.result) |
There was a problem hiding this comment.
| testRule(t, linter, dashboard, tc.result) | |
| for _, linter in []Rule{NewTargetJobRule(), NewTargetInstanceRule()} { | |
| testRule(t, linter, dashboard, tc.result) | |
| } | |
Several jobs had an anti-pattern of linting a panel, but iterating over that panels targets/queries.
The
Targetrule type exists for this purpose, and theRuleSetiterates over them externally.This PR migrates those panel rules to target rules and cleans up a couple minor things along the way.