add autofix option

[zeitlinger]

• for 1 rule in the beginning, to see if this is a good idea
• github.com/miracl/conflate has been forked - I'll create a PR upstream if this PR is accpeted - see miracl/conflate@master...zeitlinger:conflate:merge_slice

[rgeyer]

This looks great. It looks like you implemented auto-fix on the template-on-time-change-reload-rule?

There are a few things which we should consider changing before adding this functionality.

1. The rules are intended to operate with as little context as possible. I.E. They would be given everything they need in parameters, such as a dashboard and panel reference for a panel rule. Rules should not require data about configuration as that's too much context.

I'd like to see the rules return a func reference in the Result struct (or something similar) which could then be executed by the caller. This would allow for the configuration to opt-out of auto-fix for a subset of rules.

2. Because of 1 above, rules would need to be able to return more than a single result. This was an early design decision, where I assumed that rules would only ever find one issue and report it, but in practice some rules can find multiple issues. As such, the interface for rules should be changed to allow for multiple Result from the same rule.

If we implement 1 as I suggested (a "fix" func on the Result) this would allow the caller/ResultSet (which has all of the config context) to decide whether to execute auto-fix or not.

3. Many of these can not be "fixed" as they'd require the user to make informed decisions about their dashboard. For those, presumably we just wouldn't implement auto-fix.

[zeitlinger]

I think I get the idea - it's a good separation of concerns.

I've written just one method to make sure this is going into the right direction. Is that what you have in mind?

func (f TargetRuleFunc) Lint(d *Dashboard, s *ResultSet) {
	for pi, p := range d.GetPanels() {
		p := p // capture loop variable
		for ti, t := range p.Targets {
			t := t // capture loop variable
			results := f.fn(*d, p, t)
			var rr []Result

			for _, r := range results.Results {
				var fix func(*Dashboard)
				if r.Fix != nil {
					fix = func(dashboard *Dashboard) {
						r.Fix(*d, p, &t)
						p.Targets[ti] = t
						d.Panels[pi] = p
					}
				}
				rr = append(rr, Result{
					Severity: r.Severity,
					Message:  r.Message,
					Fix:      fix,
				})
			}
			s.AddResult(ResultContext{
				Result:    RuleResults{rr},
				Rule:      f,
				Dashboard: d,
				Panel:     &p,
				Target:    &t,
			})
		}
	}
}

[rgeyer]

I think I get the idea - it's a good separation of concerns.

I've written just one method to make sure this is going into the right direction. Is that what you have in mind?

func (f TargetRuleFunc) Lint(d *Dashboard, s *ResultSet) {
	for pi, p := range d.GetPanels() {
		p := p // capture loop variable
		for ti, t := range p.Targets {
			t := t // capture loop variable
			results := f.fn(*d, p, t)
			var rr []Result

			for _, r := range results.Results {
				var fix func(*Dashboard)
				if r.Fix != nil {
					fix = func(dashboard *Dashboard) {
						r.Fix(*d, p, &t)
						p.Targets[ti] = t
						d.Panels[pi] = p
					}
				}
				rr = append(rr, Result{
					Severity: r.Severity,
					Message:  r.Message,
					Fix:      fix,
				})
			}
			s.AddResult(ResultContext{
				Result:    RuleResults{rr},
				Rule:      f,
				Dashboard: d,
				Panel:     &p,
				Target:    &t,
			})
		}
	}
}

Yes! This is exactly right. This would allow us to conditionally execute the fix, and allow one rule to provide multiple results/fixes.

[zeitlinger]

@rgeyer finally found the time - please have a look 😄

[rgeyer]

Great work! Thank you for this contribution!

This PR is quite large, so I won't ask that you add it here, but.. Can you add some brief documentation on the autofix CLI flag, and what the outcome will be?

This should be in the docs/ path. Specifically, it would be useful to add detail to docs/rules/template-on-time-change-reload-rule.md which explains what happens if autofix is executed for this rule.

Thank you again! 🎉 🚀

[rgeyer]

Good abstraction here. I can see value in keeping the original definition of the data source, not only for marshalling into the struct as you do with GetDataSource but potentially also for other validation/conversion later. 👍

[zeitlinger]

it was a necessity actually - to keep the marshall/unmarshall symetric

[rgeyer]

These are great helpers which will ensure continuity in the error/warning messages. Very good. 🎉

[tomwilkie]

Hi @zeitlinger & @rgeyer - I reverted this due to #136, thought it best to revert first and ask questions later. Please do get this merged again ASAP, once we've figure out what to do about the replace. Thanks!

[zeitlinger]

Hi @zeitlinger & @rgeyer - I reverted this due to #136, thought it best to revert first and ask questions later. Please do get this merged again ASAP, once we've figure out what to do about the replace. Thanks!

Yes, absolutely.
I'll check out the problem now.

[zeitlinger]

This should be in the docs/ path. Specifically, it would be useful to add detail to docs/rules/template-on-time-change-reload-rule.md which explains what happens if autofix is executed for this rule.

it's in c9518b2, which is part of #138

[zeitlinger]

Yes, absolutely. I'll check out the problem now.

Here's the new PR: #138