logcheck: support settings in golangci-lint #18
Conversation
This implements the new interface from golangci/golangci-lint#3887 with two fields in the "settings": type settings struct { Check map[string]bool `json:"check"` Config string `json:"config"` } This is the actual config *data*, not the name of a config file. This is intentional because - relative file paths wouldn't work (plugin cannot resolve them) - loading additional file content wouldn't invalidate cached linter results the that embedding the data inside the main config does The `include` text template method ensures that logcheck.conf gets injected automatically. Having it as separate file has the advantage that manual invocations of the stand-alone logcheck linter still work.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
|
/hold golangci/golangci-lint#3887 should get merged first, to ensure that the new plugin API doesn't change anymore. |
k8s-ci-robot
added
the
do-not-merge/hold
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
| // | ||
| // The downside is that format errors are less user-friendly. | ||
| var buffer bytes.Buffer | ||
| if err := json.NewEncoder(&buffer).Encode(pluginSettings); err != nil { |
There was a problem hiding this comment.
If we don't mind adding a third dependency of "github.com/mitchellh/mapstructure", it's simpler the parse pluginSettings.
There was a problem hiding this comment.
golangci-lint already depends on it, so it wouldn't make a difference for Kubernetes in hack/tools. But here I prefer to avoid such a heavy dependency.
I am also not sure whether mapstructure would provide better error messages. Does it?
There was a problem hiding this comment.
mapstructure provides detailed error messages but I think it is most useful for decoding purposes.
This fixes random parser failures.
k8s-ci-robot
added
size/L
|
/hold cancel The new API is going to be supported by golangci-lint in a new release expected tomorrow, and that release will warn about logcheck when it only supports the old API. We should merge this PR and use the updated logcheck in Kubernetes. |
k8s-ci-robot
removed
the
do-not-merge/hold
|
A test PR with this modified logcheck is here: kubernetes/kubernetes#119860 |
Namanl2001
left a comment
Namanl2001
left a comment
There was a problem hiding this comment.
on a higher level, the changes looks good to me
|
@Namanl2001: are you confident enough with the changes to do a formal LGTM? |
yes /lgtm |
k8s-ci-robot
added
the
lgtm
5 participants
This implements the new interface from
golangci/golangci-lint#3887 with two fields in the "settings":
This is the actual config data, not the name of a config file. This is intentional because
The
includetext template method ensures that logcheck.conf gets injected automatically. Having it as separate file has the advantage that manual invocations of the stand-alone logcheck linter still work.