-
Notifications
You must be signed in to change notification settings - Fork 24
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
upgrade golangci-lint to 1.52 #127
Conversation
lint/rule_target_job_instance.go
Outdated
@@ -12,7 +12,7 @@ func newTargetRequiredMatcherRule(matcher string) *TargetRuleFunc { | |||
name: fmt.Sprintf("target-%s-rule", matcher), | |||
description: fmt.Sprintf("Checks that every PromQL query has a %s matcher.", matcher), | |||
fn: func(d Dashboard, p Panel, t Target) Result { | |||
// TODO: The RuleSet should be responsible for routing rule checks based on their query type (prometheus, loki, mysql, etc) |
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.
I presume that the TODOs were removed as a result of the updated linting? Can we override that rule?
All of the TODOs which were removed are still not complete, and.. I've been a poor maintainer in that they aren't tracked as issues either 🤦
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.
yes, we can override that rule - I'll do that
One question on the TODOs. I approved pre-maturely assuming that this only upgraded the linter, without necessarily having any code changes. |
When attempting to get the configured unit of a panel, there's a chance that the field configuration is not on the dashboard definition. We should nil check this property before we attempt to access it. Looks like this was introduced in #127
Based on #126