Enable golint with golangci-lint

[elfosardo]

Most linters are disabled by default when using golangci-lint, we need
to explicitely enable them if we want them to run.
This change enables the basic golint support.
For more info please check https://golangci-lint.run/usage/quick-start

[elfosardo]

golint should now fail

[elfosardo]

@dhellmann golint and govet now fail, not sure which options we want to keep
if we leave golint, we need to fix the rest besides what's fixed in #823

[dhellmann]

I think all of the failures are things we should fix, so I'm OK with the set of rules that are enabled.

The govet and golint jobs are running the same thing. Is govet enabled? We could probably drop one of the jobs if the same tool runs all of the tests in one job.

[elfosardo]

I think all of the failures are things we should fix, so I'm OK with the set of rules that are enabled.

The govet and golint jobs are running the same thing. Is govet enabled? We could probably drop one of the jobs if the same tool runs all of the tests in one job.

yes, govet is included in golint
I agree we should drop govet and just run golint, I don't have that power though :)
I'm going to fix the rest of the lint errors here

[elfosardo]

I will leave vet in makefile until govet job has been removed

[elfosardo]

/test-integration

[elfosardo]

/assign @dhellmann @hardys

[fmuyassarov]

I will leave vet in makefile until govet job has been removed

If the plan is to remove the govet from mandatory CI jobs, that can be done here https://github.com/metal3-io/project-infra/blob/ebe2d90c26bb3bf284fd34c2c171a82058249da6/prow/config/config.yaml#L252-L265

[elfosardo]

I will leave vet in makefile until govet job has been removed

If the plan is to remove the govet from mandatory CI jobs, that can be done here https://github.com/metal3-io/project-infra/blob/ebe2d90c26bb3bf284fd34c2c171a82058249da6/prow/config/config.yaml#L252-L265

oh cool, thanks for pointing that out!
I will submit a patch for that, but I think we should merge this first

[dtantsur]

What's with this p vs m in methods? It doesn't seem like a meaningful change to me, and it makes this patch conflict with everything other patch in existence. Maybe skip that?

[elfosardo]

What's with this p vs m in methods? It doesn't seem like a meaningful change to me, and it makes this patch conflict with everything other patch in existence. Maybe skip that?

@dtantsur it's unfortunately a valid lint error:
pkg/provisioner/demo/demo.go:81:1: receiver name p should be consistent with previous receiver name m for demoProvisioner

we never really ran golint here that's why now we need to make these changes

[dhellmann]

One nit, but I'm not sure it really matters.

Thanks for working on this!

/approve

[dhellmann]

I think this was using fmt instead of fmt-check so that code would be fixed before being passed to the compiler.

[elfosardo]

I think fmt-check is correct as it's using the gofmt.sh script under /hack
fmt was just an alias for lint
vet is also an alias, but we will remove it later to not break the current jobs

[dhellmann]

/test-integration

[dhellmann]

oh, now there are merge conflicts

[elfosardo]

/test-integration

[elfosardo]

/assign @asalkeld

[dhellmann]

/approve
/lgtm

Let's get this in so we don't regress the lint job and have to keep fixing issues in this PR.

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, elfosardo

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:

• OWNERS [dhellmann]

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

[elfosardo]

@dhellmann not sure why this is "Not mergeable", CI seems ok now :/

[dhellmann]

There are some issues with the upstream Prow instance.

@maelk @kashifest @zaneb is it safe for me to merge this PR by hand?

[kashifest]

There are some issues with the upstream Prow instance.

@maelk @kashifest @zaneb is it safe for me to merge this PR by hand?

OK for me. By the way since tide is still working, we tested IPAM repo with enabling Travis to do a the tests and making prow optional and tide was able to merge it. We configured Travis test as required now. I have a PR to do the same for CAPM3 and Dev-Env. Shall I do the same for BMO? metal3-io/project-infra#179. You have to configure Travis tests as required.

[dhellmann]

There are some issues with the upstream Prow instance.
@maelk @kashifest @zaneb is it safe for me to merge this PR by hand?

OK for me. By the way since tide is still working, we tested IPAM repo with enabling Travis to do a the tests and making prow optional and tide was able to merge it. We configured Travis test as required now. I have a PR to do the same for CAPM3 and Dev-Env. Shall I do the same for BMO? metal3-io/project-infra#179. You have to configure Travis tests as required.

It sounds like that's probably a good idea, but we should talk about it somewhere that more community members will be able to offer their opinion. Maybe the mailing list?

In the mean time, I'll merge this one by hand.