Kubebuilder: [Discussion] Use golangci.com for linting instead of running it in Travis

[Adirio]

GolangCI offers a SaaS platform to run linters. This issue aims to discuss the advantages and drawback of using such SaaS instead of running ourselves the test in Travis.

Note: this issue only focuses on linting tests, it is not related to any other test run in Travis.

Features:

1. Free for open source projects (link).
2. Configurable in .golangci.yaml file (link).
3. Badges not yet implemented (link).
4. Bot that comments linting errors in PRs, providing fixes if able.
5. Test for PRs.
6. Analysis reports.

[Adirio]

I think that moving from Travis towards golangci.com may be a good idea as it will add some useful features while not loosing anything:

Same behavior:

• Both options are free for Open Source projects, so no drawback there.
• Both options use .golangci.yaml file to configure which linters should be enabled (#1365). This file can be extended in golangci.com to configure how the test is run, which would be equivalent to modifying scripts/verify.sh.
• Tests that can be marked as required if needed.
• All the console output we can see at Travis can also be seen at the reports provided by golangci.com.

Additional features:

• Bot that takes care of reviewing linting issues, reducing the work a reviewer needs to do.

@droot @DirectXMan12 @mengqiy @camilamacedo86 @alexeldeib

[alexeldeib]

Isn’t macOS and Windows support the real reason for Travis? If anything, I’d hope we can reduce our matrix (e.g if prow magically supported all three).

Maybe it’s worth doing something weird like configuring tide/all the merge goodies but using Travis for all of the actual tests? I wonder if we can link prow to Travis test results?

I heard some tell of changes to how the check system works, maybe we can get github to automate more of this for us somehow?

[Adirio]

@alexeldeib

Isn’t macOS and Windows support the real reason for Travis? If anything, I’d hope we can reduce our matrix (e.g if prow magically supported all three).

This is just about the linting part. Linting is OS-agnostic.

[camilamacedo86]

Hi @Adirio,

IMO the lint check needs to be done in order to allow or NOT the merge of any PR. ( this is only my concern and I am happy with any change that still ensuring it )

WHY?

• If we do the check and allow merge any way has no need to do it
• And then, we will allow in the 99% of the cases the introduction of a bad implementation which were already checked and notified by the lint.

According to @mengqiy and @DirectXMan12, they have good reasons to do it in Prow and then, I am cool with that too 👍 .

Over this topic

• What is the @Adirio proposal: Use https://golangci.com
• What are the advantages according to him?

The main advantage will be that a bot will comment on the linting issues without reviewers having to check them

IHMO: It shows cool 👍 and I am OK with. +1 to give a try at least.

IMPORTANT The lint checks should NOT/ can NOT be used to ensure if the project works in the SO X or Y ( Mac or Linux ). It is not its purpose and has no relation with.

c/c @alexeldeib

[Adirio]

I tried to set it up in my own repository. It was really trivial.

1. Visit https://golangci.com and login with GitHub.
2. Grant access to public repositories.
3. Press Connect button in the repository list.
4. Extra step: set up a rule that marks this test as Required.
   
   
   
   1. Go to Settings tab.
   
   
   2. Go to Branches menu option.
   
   
   3. Press Add rule button.
   
   
   4. Set Branch name pattern to either * or master.
   
   
   5. Check Require status checks to pass before merging.
   
   
   6. Check Require branches to be up to date before merging.
   
   
   7. Check GolangCI.
   
   

Some images that may help

https://golangci.com/repos/github:

Mark tests as required:

Result: Adirio/kubebuilder#2

[alexeldeib]

I know it’s about the linting, but we are increasing our tooling matrix for little reason imo. I really like the free golangci lint service, so maybe I’m just nitpicking, but it feels like it’s not really that much better than running the CLI in one of our two existing options.

[Adirio]

I know it’s about the linting, but we are increasing our tooling matrix for little reason imo. I really like the free golangci lint service, so maybe I’m just nitpicking, but it feels like it’s not really that much better than running the CLI in one of our two existing options.

I thought the same way at the start, but we are already going to be using .golangci.yaml as the configuration once #1365 gets merged so basically golanci.com service will mean we remove one script but we don't have to configure anything else, and we win the added benefit of the bot that helps reviewing process.

[mengqiy]

The golangci bot looks really cool.
I don't have much concern as long as it meets the following:

• fast to start and fast to run
• stable
• enough visibility for contributors
• friendly enough to trigger re-run (at least should not worse than travis)

[Adirio]

• fast to start and fast to run

The test I did in https://github.com/Adirio/kubebuilder/pull/2 was instantly started and fast to run. Anyway, it will be much faster than any other test we are running so it will never be the bottleneck.

• stable

The software layer will probably be more stable than any we can use as it will be updater more often. The SaaS layer stability is hard to measure without trying.

• enough visibility for contributors

The report is exactly the same, it will actually be more accessible as the GitHub link will directly link to it instead of having to do to Travis and then go to the linting instance. And the bot wil greatly increase the visibility.

• friendly enough to trigger re-run (at least should not worse than travis)

Pushes trigger re-runs. Closing and opening will also retrigger, but this should not be needed as linting results will be the same if code doesn't change.

[toonsevrin]

It seems like this golangci requires the bot account to have admin privileges on the repository. Is this not a concern?

EDIT: I've played around a little bit and it seems like admin priviledges are only required to "connect" your repository. This will setup the webhooks. After the webhooks are setup you can downgrade to "write" permissions.

[Adirio]

Write permissions should be perfectly fine. I mean, it needs to write the code suggestions and report so write is needed.

[camilamacedo86]

Hi @Adirio,

Besides it is a good idea, not that:

See: https://medium.com/golangci/golangci-com-is-closing-d1fc1bd30e0e

So, I understand that we can close this one since it will no longer be possible.

[Adirio]

Yeah, I just received an email from them :( Sad to hear it.

[toonsevrin]

Write permissions should be perfectly fine. I mean, it needs to write the code suggestions and report so write is needed.

90% certain you don't need write permission for this. So if anyone is ambitious, they can still write a simple bot for this (instead of using golangci), kubebuilder would just have to add a webhook :)