Fix and enforce linter #283
Conversation
This adds the scafolding for running golangci-lint. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>
jacobweinstock
added
the
kind/cleanup
Codecov Report
@@ Coverage Diff @@
## main #283 +/- ##
===================================
- Coverage 37% 37% -1%
===================================
Files 41 41
Lines 2787 2782 -5
===================================
- Hits 1037 1032 -5
- Misses 1680 1683 +3
+ Partials 70 67 -3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
fantastic!. A couple of minor comments and one nit. The nit is that you're mixing 2 different ways to define functions whose arguments are all ignored. Some changes you're doing here are like:
func(_ type1, _ type2, _ type3)and
func(type1, type2, type3)I prefer the latter and I think its the most common way. Can you make it so only one way is used please?
| @@ -123,14 +123,15 @@ func (f *Finder) HasActiveWorkflow(ctx context.Context, hwID client.HardwareID) | |||
|
|
|||
| wfContexts := []*workflow.WorkflowContext{} | |||
| for _, wf := range stored.Items { | |||
| wf := wf | |||
There was a problem hiding this comment.
this was definitely buggy before
| @@ -68,5 +66,5 @@ func TestMain(m *testing.M) { | |||
| defer l.Close() | |||
| mainlog = l.Package("main") | |||
| metrics.Init(l) | |||
| os.Exit(m.Run()) | |||
| os.Exit(m.Run()) //nolint:gocritic // this seems to be the correct pattern | |||
yeah, seems reasonable. updated. |
|
:( I always forget to check of the "viewed" button while reviewing and then need to go through the whole thing again later ... |
installers/customipxe/errors.go
Outdated
|
|
||
| import "errors" | ||
|
|
||
| var ErrEmptyIpxeConfig = errors.New("ipxe config URL or Script must be defined") |
There was a problem hiding this comment.
Another nit, one that wasn't caught by gometalinter but is in the spirit of one of its linters: This should be ErrEmptyIPXEConfig since its iPXE because PXE is an acronym like DNS, ID, IP, ... and should be all caps. The i should also be caps because its a new word. It would also match other names in this repo like j.IPXEScriptURL. It'd be nice to get it fixed now.
There was a problem hiding this comment.
yeah, that's interesting that the linter didn't catch that. I've updated it regardless.
There was a problem hiding this comment.
wait what... that was way too fast. I'm pretty sure github sent that to you before I submitted it because it complained about needing a comment for the review as a whole...
This gets the repo to a baseline of linting based on the .golangci.yml file. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>
Description
This gets the repo to a baseline of linting based on the .golangci.yml file generated from running lint-install.
Why is this needed
Fixes: #63
How Has This Been Tested?
How are existing users impacted? What migration steps/scripts do we need?
Checklist:
I have: