fix some linting failures and configure golangci-lint

[dklimpel]

Checklist

• make test-all (UNIX) passes. CI will also test this
• unit and/or integration tests are included (if applicable)
• documentation is changed or added (if applicable)

Description of change

Fix some linting failures and add new go linting to makefile.

I am not a go expert. Please have a deeper look inside.

See a list of lint failures at:

• Add github golang lint CI pipeline #899

[aelsabbahy]

Thanks for adding this, l wonder if CI should run make lint to be consistent with local dev story.

For example, this PR is currently passing the golangci-lint, but failing Travis do to the gofmt check. Ideally, GitHub actions would catch both.

[aelsabbahy]

these should probably bubble up the error:

return false, err

[aelsabbahy]

Maybe we change this to also run golangci-lint.

lint: golangci-lint

[dklimpel]

I think golint does not exist anymore and golint can be replaced by golangci-lint.

[aelsabbahy]

Hah, didn't even notice that, yeah.. we can replace golint in the lint target with golangci-lint that makes more sense.

So much has changed in the go ecosystem since Goss was originally written.

[dklimpel]

For example, this PR is currently passing the golangci-lint, but failing Travis do to the gofmt check. Ideally, GitHub actions would catch both.

I have added fmt to pipeline.
Github currently does not find it because it only looks at modified code, and this line is currently not included in the diff. If you check everything, the pipeline fails always until all errors are fixed. A bit of a dilemma.

goss/.github/workflows/golangci-lint.yaml

Line 27 in 7ca5459

only-new-issues: true

[dklimpel]

I do not not knwo why it is failing.

Perhaps a Travis CI problem :(

[aelsabbahy]

Looks great, small comment on golint and make targets.

The travis-ci is failing because cmd.Run() will return an error on non-zero exit status. So would need to either:

1. Ignore the errors and skip linting those calls
2. Add a check for ExitError and ignore it
3. Refactor cmd.Run() to ignore ExitError and the consumer code has to do a check for Status if they care.

[aelsabbahy]

Hah, didn't even notice that, yeah.. we can replace golint in the lint target with golangci-lint that makes more sense.

So much has changed in the go ecosystem since Goss was originally written.

[aelsabbahy]

I think we can promote this out of draft.

At some point, I'll run a golangci-lint run and fix all the findings.

curious, what are the pros/cons of using golangci-lint-action vs just running make lint in a github action?

I assume the former provides some unique benefits/flags, but requires making sure the local make lint and the job are in sync, (same version, same flags, etc.).

Anyways, correct me if I'm wrong, but putting everything behind make should make it easier to move off of github if that ends up being another transition in the future?

[dklimpel]

The issue was:

resource/addr.go:54:49: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions (staticcheck)
	ctx := context.WithValue(context.Background(), "id", a.ID())

[dklimpel]

Looks great, small comment on golint and make targets.

The travis-ci is failing because cmd.Run() will return an error on non-zero exit status. So would need to either:

1. Ignore the errors and skip linting those calls
2. Add a check for ExitError and ignore it
3. Refactor cmd.Run() to ignore ExitError and the consumer code has to do a check for Status if they care.

I have decided to "Ignore the errors and skip linting those calls"

[dklimpel]

curious, what are the pros/cons of using golangci-lint-action vs just running make lint in a github action?

I assume the former provides some unique benefits/flags, but requires making sure the local make lint and the job are in sync, (same version, same flags, etc.).

Anyways, correct me if I'm wrong, but putting everything behind make should make it easier to move off of github if that ends up being another transition in the future?

I think the github actions are actively developed further and certainly optimised for performance and caching. For this you have to maintain makefile and action twice. The github action also runs without problems on forked branches. With the makefile, each developer has to make sure that it works, which hopefully it does out of the box.

Anyways, correct me if I'm wrong, but putting everything behind make should make it easier to move off of github if that ends up being another transition in the future?

I think so, too.
The dual maintenance is currently limited to the golint version

[dklimpel]

Sorry, the PR has become a little more complicated than planned. In the end, the currently configured linting has now been fully implemented and the pipeline no longer only needs to check the diff (only-new-issues: true)

[aelsabbahy]

This looks good to me. I assume this is ready to merge.

I can enable some of the commented out lintes and fix some of the issues as follow up PRs.

marked this as approved, if you're ready for this to be merged just post a comment here and I'll merge it.

[aelsabbahy]

Minor question:

The github action also runs without problems on forked branches. With the makefile, each developer has to make sure that it works, which hopefully it does out of the box.

Why does make lint not work on forked branches in github?

[dklimpel]

marked this as approved, if you're ready for this to be merged just post a comment here and I'll merge it.

Yes, is ready for merging.

[dklimpel]

Why does make lint not work on forked branches in github?

I think I was thinking too much. The crucial point is that the Travis CI only runs when you create a PR. Otherwise you would have to have your own Travis account. But for development it would also be good to run the CI beforehand creating a PR. For example, I sometimes develop directly in GitHub without a computer that has make and co installed. It's good to get direct feedback.