search

goss-org / goss

Quick and Easy server testing/validation
https://goss.rocks
Apache License 2.0
5.5k stars 470 forks source link

fix some linting failures and configure golangci-lint #922

Closed dklimpel closed 3 weeks ago

dklimpel commented 1 month ago
Checklist

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:

dklimpel commented 1 month ago

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.

https://github.com/goss-org/goss/blob/7ca545977ec1c9c9c3de6e612b92940940be849d/.github/workflows/golangci-lint.yaml#L27

dklimpel commented 1 month ago

I do not not knwo why it is failing.

image

Perhaps a Travis CI problem :(

dklimpel commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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.