golangci added to verify target, fixes #746
Conversation
|
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: 7e0a67a 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/6169af90d15e2e0007f9e355 |
JacobGabrielson
changed the title
| run: | ||
| tests: false | ||
|
|
||
| timeout: 5m |
There was a problem hiding this comment.
you can remove the --timeout 5m arg in the Makefile now since we have this
| @@ -39,7 +39,8 @@ func (f *Filter) GetProvisionablePods(ctx context.Context, provisioner *v1alpha4 | |||
| } | |||
| // 2. Filter pods that aren't provisionable | |||
| provisionable := []*v1.Pod{} | |||
| for _, p := range pods.Items { | |||
| for i := range pods.Items { | |||
There was a problem hiding this comment.
curious why we'd switch to using i here rather than just ranging over the actual pod? We do the range over the object ignoring the index in other loops.
There was a problem hiding this comment.
Probably to avoid variations of https://dev.to/kkentzo/the-golang-for-loop-gotcha-1n35
There was a problem hiding this comment.
However, I totally agree, and don't like this indexing style.
There was a problem hiding this comment.
@ellistarn that's right about the for loop gotcha - is there a different style you'd prefer?
There was a problem hiding this comment.
@bwagner5 the difference is that in other loops, the address of the range object isn't being taken (and there's no closure going on either), so the linter isn't warning about those.
| @@ -86,7 +86,8 @@ func (r *Emptiness) isEmpty(ctx context.Context, n *v1.Node) (bool, error) { | |||
| if err := r.kubeClient.List(ctx, pods, client.MatchingFields{"spec.nodeName": n.Name}); err != nil { | |||
| return false, fmt.Errorf("listing pods for node %s, %w", n.Name, err) | |||
| } | |||
| for _, p := range pods.Items { | |||
| for i := range pods.Items { | |||
There was a problem hiding this comment.
I'm seeing this pattern in a lot of places now. Is this a lint recommendation? I'm not sure I agree that this is better. Is this supposed to protect against the pointer location changing out from under the local loop var?
There was a problem hiding this comment.
Only when it's closed over or its address is taken.
| // isNotFound returns true if the err is an AWS error (even if it's | ||
| // wrapped) and is a known to mean "not found" (as opposed to a more | ||
| // serious or unexpected error) | ||
| func isNotFound(err error) bool { |
There was a problem hiding this comment.
consider making this public
There was a problem hiding this comment.
Just curious why? I kinda got the impression public-ness was a signal that the function would be used outside of the aws package?
There was a problem hiding this comment.
I'd agree on keeping this private. Limits the IDE-oopsy of importing this into another pkg that's not in aws cloud provider
| @@ -0,0 +1,33 @@ | |||
| # See https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml | |||
There was a problem hiding this comment.
I'd trust the knative folks to have landed on a set of sensible defaults: https://github.com/knative-sandbox/sample-controller/blob/main/.golangci.yaml. WDTY?
There was a problem hiding this comment.
I started with those, but they had chosen a few that weren't a good idea. For example, one of them exhorted one to use make to pre-allocate slices. Which, while sometimes a good idea, can be a pretty dangerous change to make without a lot of testing as the semantics do change subtly.
| - unparam | ||
| disable: | ||
| - errcheck | ||
| - prealloc |
There was a problem hiding this comment.
This is the only field that diverges from kn IIUC
There was a problem hiding this comment.
See my other comment about why prealloc isn't a good idea.
1. Issue, if available:
N/A
2. Description of changes:
Per @ellistarn add golangci config; fix errors found by same
3. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.