add golangci-lint #600

runcom · 2019-04-03T11:42:08Z

Signed-off-by: Antonio Murdaca [email protected]

- What I did

Add https://github.com/golangci/golangci-lint to our verify target and fix all the current warnings to start fresh once this PR merges. The linter binary is already go get'ed with openshift/release#3332

closes #447

- How to verify it

CI

- Description for the changelog

runcom · 2019-04-03T11:43:11Z

/hold

since there a bunch of stuff to be fixed

runcom · 2019-04-03T13:20:22Z

oh cool, verify it's working already 😍

kikisdeliveryservice · 2019-04-03T16:09:00Z

i love this!!

cgwalters · 2019-04-03T16:35:30Z

Oh hum...we probably should have noticed the MCO was using md5 earlier 😢
Not totally sure if we can change it now easily.

runcom · 2019-04-03T16:37:34Z

Yeah, I'm not sure about even landing this for 4.1 due to the many stuff to take care of but still. Now we noticed and we can think about migrating/fixing some of this stuff.

runcom · 2019-04-04T08:45:43Z

made another pass at this, changes are trivial though

runcom · 2019-04-04T09:07:15Z

Removed WIP, some linters are disabled or this PR becomes a never ending PR. After this merges, I'll open an issue for contributors and us to re-enable and fix the remaining disabled linters.

Still on hold cause #603 since it's the last error for this

ashcrow · 2019-04-04T13:38:42Z

golangci-lint run
2019/04/04 09:20:21 unknown architecture wasm
pkg/controller/kubelet-config/kubelet_config_controller.go:335:67: `(*Controller).syncStatusOnly` - `err` is unused (unparam)
func (ctrl *Controller) syncStatusOnly(cfg *mcfgv1.KubeletConfig, err error, args ...interface{}) error {

runcom · 2019-04-04T13:41:19Z

golangci-lint run
2019/04/04 09:20:21 unknown architecture wasm
pkg/controller/kubelet-config/kubelet_config_controller.go:335:67: (*Controller).syncStatusOnly - err is unused (unparam)
func (ctrl *Controller) syncStatusOnly(cfg *mcfgv1.KubeletConfig, err error, args ...interface{}) error {

#600 (comment)

runcom · 2019-04-10T10:32:50Z

This requires #618 in order to be consistent

/hold

runcom · 2019-04-10T14:01:35Z

/hold

droslean · 2019-04-10T15:10:54Z

/test verify

runcom · 2019-04-10T15:15:39Z

/test verify

this is not gonna pass as we aren't and we won't be at 1.11 for now

runcom · 2019-05-15T11:05:02Z

depends on openshift/release#3799

DanyC97 · 2019-05-15T17:25:24Z

Makefile

@runcom qq - is it bound to a particular release ?

i suspect it is by looking https://github.com/openshift/release/pull/3799/files so maybe the echo can be extended to mention that ... my 0.02$ though

closed that PR as it's not needed now that we're all above go1.10 (1.12 to be precise)

How about adding a target to install it for you? e.g.

$(GOBIN)/golangci-lint: go get -u github.com/golangci/golangci-lint/cmd/golangci-lint verify: $(GOBIN)/golangci-lint

I've gona back and forth on the target (we used to have it in CRI-O as well but ended up being dropped cause the CI was taking care of it https://github.com/cri-o/cri-o/pull/2247/files#diff-b67911656ef5d18c4ae36cb6741b7965L308)

The real pain with go1.12 and onwards would be modules also (and calculating $GOBIN). I'm not yet familiar with that. I'm super fine following up to this and add targets to install tools also 👍

👍 I guess I'm used to having a golang version check to handle compatibility. there were issues with GOBIN? never come across that myself.

I guess I'm used to having a golang version check to handle compatibility.

oh cool, that might be something useful to have here in MCO as well :)

there were issues with GOBIN? never come across that myself.

afaict with go modules, go build creates binaries in PWD so that must be conditionalized as well (hence your suggestion above to version check is amazing)

runcom · 2019-06-12T09:43:21Z

This is good to go now

runcom · 2019-06-12T09:45:39Z

/hold cancel

sgreene570 · 2019-06-12T16:32:58Z

.golangci.yml

we definitely want to exclude unit test files from linting? Just double checking.

yep, but as with the other "ignored" linters in this file, we might want, at some point, remove this and fix everything. Right now we need to get this in and fix code (not tests) to have this run and check on every PR w/o asking ppl to gofmt as you noticed.

ok sounds good

Fix all the current warnings as well Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2019-06-13T13:48:06Z

rebased again 😄

kikisdeliveryservice · 2019-06-13T16:14:55Z

/lgtm

openshift-ci-robot · 2019-06-13T16:15:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kikisdeliveryservice, runcom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [kikisdeliveryservice,runcom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 3, 2019

openshift-ci-robot requested review from LorbusChris and kikisdeliveryservice April 3, 2019 11:42

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2019

runcom force-pushed the linters branch from 505c80a to 36e8164 Compare April 3, 2019 12:30

runcom force-pushed the linters branch from 36e8164 to 829a18b Compare April 4, 2019 08:45

runcom force-pushed the linters branch 2 times, most recently from 5108719 to db00105 Compare April 4, 2019 08:57

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 4, 2019

runcom force-pushed the linters branch 2 times, most recently from 9ab5d36 to 9b4374b Compare April 4, 2019 09:06

runcom changed the title ~~WIP: add golangci-lint~~ add golangci-lint Apr 4, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2019

runcom force-pushed the linters branch from 9b4374b to 7ed1c92 Compare April 4, 2019 09:18

runcom force-pushed the linters branch from 7ed1c92 to b8ccf9a Compare April 5, 2019 10:07

runcom mentioned this pull request Apr 10, 2019

Dockerfile.*: bump to go1.11 #618

Closed

runcom force-pushed the linters branch from b8ccf9a to b4b37cd Compare April 10, 2019 10:32

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 10, 2019

runcom mentioned this pull request Apr 12, 2019

*: pre-transition ign refactor #610

Merged

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2019

runcom force-pushed the linters branch from 218309e to 7781c74 Compare May 15, 2019 10:47

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2019

runcom force-pushed the linters branch from 7781c74 to a6fc45e Compare May 15, 2019 10:49

DanyC97 reviewed May 15, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2019

runcom mentioned this pull request Jun 7, 2019

Standardize import paths and run gofmt #831

Merged

runcom force-pushed the linters branch from a6fc45e to b28d023 Compare June 12, 2019 09:37

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 12, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2019

runcom removed the 4.2 label Jun 12, 2019

sgreene570 reviewed Jun 12, 2019

View reviewed changes

add golangci-lint

f4eb9ec

Fix all the current warnings as well Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the linters branch from b28d023 to f4eb9ec Compare June 13, 2019 13:47

openshift-ci-robot assigned kikisdeliveryservice Jun 13, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 13, 2019

openshift-merge-robot merged commit 0d843aa into openshift:master Jun 13, 2019

add golangci-lint #600

add golangci-lint #600

Uh oh!

Conversation

runcom commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Apr 3, 2019

Uh oh!

runcom commented Apr 3, 2019

Uh oh!

kikisdeliveryservice commented Apr 3, 2019

Uh oh!

cgwalters commented Apr 3, 2019

Uh oh!

runcom commented Apr 3, 2019

Uh oh!

runcom commented Apr 4, 2019

Uh oh!

runcom commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashcrow commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Apr 4, 2019

Uh oh!

runcom commented Apr 10, 2019

Uh oh!

runcom commented Apr 10, 2019

Uh oh!

droslean commented Apr 10, 2019

Uh oh!

runcom commented Apr 10, 2019

Uh oh!

runcom commented May 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Jun 12, 2019

Uh oh!

runcom commented Jun 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Jun 13, 2019

Uh oh!

kikisdeliveryservice commented Jun 13, 2019

Uh oh!

openshift-ci-robot commented Jun 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

runcom commented Apr 3, 2019 •

edited

Loading

runcom commented Apr 4, 2019 •

edited

Loading

ashcrow commented Apr 4, 2019 •

edited

Loading