Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IMP: Add golang linter for PRs without blocking #279

Draft
wants to merge 4 commits into
base: 2.7.1-dev
Choose a base branch
from

Conversation

thebeline
Copy link
Contributor

This adds the golangci-lint action for Push and Pull Request.

A few notes on the implementation in this PR:

  • Calls golangci-lint with --issues-exit-code 0, which will prevent it from blocking Pull Requests at this time.
    This will effectively make the linter/linters behave in strictly an annotation mode without impacting the Pull Request process. This will assist in the PR review process, but leave the ability to Merge or not fully in the hands on the maintainers. I would suggest that we make this blocking in the future, as the behaviour with --issues-exit-code 0 is to always show a green PASS mark, which is mildly misleading, but not my call.
  • This has a LOT of linters active, most of the example linters, with the example configurations.
    At this time, it is acceptable, but after the initial integration, it will be advisable to fine-tune these settings as the maintainers see fit.
  • There is some magic happening in the initialization of the action, this is temporary.
    See the in-line notes, this can be resolved once we bring the Repo up to speed and spec with golang1.16.x which is currently blocked by the way we are handling vendors/* and Modules. This isn't urgent, but I figured I would mention it.
  • This will only display lint errors for changes included in the PR (via patching checks)
  • I have disabled uniq-by-line because the default behaviour is to mask multiple issue on the same line (even if they are not "duplicates").
    Evidently this is expected behaviour. Which doesn't make sense to me, but 🤷‍♂️

I have run a bunch of tests on this by making PRs with limited introduced issues, I will make an example with 2.7.0 into an old master shortly.

Resolves: #278

Must be over 1.14 to trigger automatic vendoring for potential linter usage.
Should not impact building for JESSIE (testing).
@thebeline
Copy link
Contributor Author

thebeline commented Mar 29, 2021

Ignore #280

The real example is: thebeline#6

The example lint run is: https://github.com/thebeline/OctoScreen/pull/6/checks?check_run_id=2220183186

Example Annotations are at: https://github.com/thebeline/OctoScreen/pull/6/files

Yeah... By my count that would be around 1400 Lint advisements... phew...

@thebeline
Copy link
Contributor Author

The reports for gofumpt and goimports are less than helpful, I may remove them for now...

@thebeline
Copy link
Contributor Author

thebeline commented Mar 29, 2021

Example Lint after removing gofump and goimports contains fewer reports, and they are all helpful: https://github.com/thebeline/OctoScreen/pull/6/checks?check_run_id=2220287903

Now only ~950 reports, mush betterer.

Most are Line Length issues. I have it set to what we used to use in my previous job, 80 characters. This may seem short, but it ensures that full lines are readable from basically any shell or term width.

@thebeline
Copy link
Contributor Author

thebeline commented Mar 29, 2021

Things I am noticing:

  • Maybe godot scope should be toplevel instead of declarations
  • We use ALL_CAPS a lot for conts. It seems this isn't a typical golang convention. we may consider using the following above such declarations to prevent linting errors:
    //lint:ignore ST1003 We DO use ALL_CAPS for some things
    Or one of the other options for exclude to trim certain cases.

Like I said, for the time being this will not prevent PRs, and will just be for reviewer suggestions, but after we fine-tune it a bit, it will be incredibly helpful.

Additionally, I am not sure why exactly, but in some cases the godot linter treated commented out includes as "comments" and produced a lint error, and in (many) other cases it did not. I wonder why that is...

If I were to guess, I would guess that there were too many annotations/hits, and it chunked out. It would be uncommon for the linter to have to run on such a large file, so not really a concern...

@thebeline
Copy link
Contributor Author

Last note: It should be mentioned that this also includes the IMP/ModStub branch. This is one of the things that was needed for.

@thebeline thebeline marked this pull request as draft April 15, 2021 11:39
@thebeline
Copy link
Contributor Author

@JeffB42 - Hrm... This is what killed me last year... I went down some sort of rabbit hole contributing to the linter repo and got all side-tracked. I need to round back to where I was and see what gives. I'll get back to this one shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant