Add linters to makefile/github actions #787
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
Note this won't pass yet, because we need to fix the issues (hence the stream of small PRs). /hold |
k8s-ci-robot
added
the
do-not-merge/hold
| path: git-sync | ||
|
|
||
| - name: golangci-lint | ||
| uses: golangci/golangci-lint-action@v3 |
There was a problem hiding this comment.
Should we copy the k/k .golangci-lint config?
There was a problem hiding this comment.
Sure, I can pull it in :-) Might as well have a consistent style.
| working-directory: git-sync | ||
| version: v1.53.3 | ||
|
|
||
| - name: make lint |
There was a problem hiding this comment.
why make lint AND golangci-lint-action ?
There was a problem hiding this comment.
Ah, I'll add a comment. It's because the github action is supposed to format the output such that github will then put comments onto the right lines of the PR. So I added it first. But generally I think we want to be able to run things outside of github actions, hence I consider it a more friendly first-pass on the "canonical test", which is make lint.
Of course we do now (potentially) have two sources of truth, and some linter warnings (the ones from gostaticcheck) will be presented in the less friendly way. So not sure whether this is a win or not. I figured we could try it, but I'm flexible. WDYT?
There was a problem hiding this comment.
I am fine with make lint, but iwhy would they produce different results?
There was a problem hiding this comment.
Well make lint runs two linters, and the github action form for the second linter didn't look as useful.
But my bigger concern is that while I think they do edit: the two invocations of the first linter do the same thing today, as we add more options and evolve, drift is possible. If we weren't "wearing two watches", drift wouldn't be possible.
It's probably a balancing act between how helpful the "pretty" warnings are vs the drift risk over time. And I suspect we can just evaluate that from experience. Thanks for lgtm :-)
|
#786 merged |
justinsb
force-pushed
the
add_linter_to_github_actions
branch
from
5e1527a
to
826f9ea
Compare
k8s-ci-robot
added
size/M
Not yet ready to make gating, but we can watch the progress.
Adapted the "strict" settings, but removed a few exclusions that aren't relevant here (e.g. zz_generated), and removed the custom logcheck linter (as we aren't currently using structured logging).
justinsb
force-pushed
the
add_linter_to_github_actions
branch
from
826f9ea
to
914067f
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, thockin 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
approved
thockin
removed
the
do-not-merge/hold
Builds on #786, adds a linter task to the makefile and a task to the github actions CI.
I think if we merge #786, then this will run in github actions CI.
Issue #777