Skip to content

Add the golangci-lint GitHub action#6537

Merged
jetstack-bot merged 1 commit intocert-manager:masterfrom
wallrj:golangci-lint
Dec 12, 2023
Merged

Add the golangci-lint GitHub action#6537
jetstack-bot merged 1 commit intocert-manager:masterfrom
wallrj:golangci-lint

Conversation

@wallrj
Copy link
Copy Markdown
Member

@wallrj wallrj commented Dec 7, 2023 •
edited
Loading

Run golangci-lint as a GitHub action, to provide fast feedback to a PR author about various common mistakes that are found in their code.

Initially we enable only one linter and check only G112.
The idea is that other checks and linters can be enabled one-by-one in separate PRs.
Each PR will enable only one new check and fix any offending code.
This way, the PRs are easy to review
and easy to cherry-pick into previous release branches if the fixes are deemed important enough.

Eventually, when all standard linters and checks have been enabled, the non-standard file .golangci-lint.*ci*.yaml can be renamed to .golangci-lint.yaml so that it developers can simply run golangci-lint run . locally (or via their editor / lsp-server) and get feedback before even creating a PR>

The advantages of running golangci-lint via a GitHub Action vs running it via prow + make target, are:

  1. Performance: The GitHub action has been optimized to report as fast as possible, by careful caching and parallelization: https://github.com/golangci/golangci-lint-action#performance. It is possible to achieve the same results via Prow but it would take time and effort to achieve and maintain.
  2. Web UI: Any failures are reported as GitHub Actions annotations which show up alongside the code changes in each PR. This makes it very easy to see what code needs changing. It would not be possible to achieve this using Prow.

The disadvantages are:

  1. The cert-manager maintainers will need to have expertise in Prow and GitHub Actions.

Instances of G112 have already been fixed in a another PR:

NONE

/kind cleanup

@jetstack-bot jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 7, 2023
@wallrj wallrj mentioned this pull request Dec 7, 2023
Closed
@wallrj
Add the golangci-lint GitHub action
70cf0d2 
Initially we enable only the gosec linter and only check G112
because that has been addressed in cert-manager#6534.

Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj wallrj mentioned this pull request Dec 7, 2023
Merged
@wallrj
Copy link
Copy Markdown
Member Author

wallrj commented Dec 11, 2023

/kind cleanup

@jetstack-bot jetstack-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Dec 11, 2023
@wallrj wallrj changed the title WIP: Add the golangci-lint GitHub action Add the golangci-lint GitHub action Dec 11, 2023
@jetstack-bot jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2023
@wallrj wallrj requested a review from maelvls December 11, 2023 16:31
@wallrj
Copy link
Copy Markdown
Member Author

wallrj commented Dec 11, 2023

/hold until discussed with other maintainers.

@jetstack-bot jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 11, 2023
@maelvls
Copy link
Copy Markdown
Member

maelvls commented Dec 12, 2023

Hey, I am very happy that we are finally going to try this approach. As long as people interacting with the project are able to quickly figure out how to run the same checks locally (documentation or make command), I am totally OK having linters and checks outside of Prow.

/lgtm
/hold if someone else wants to review

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2023
@inteon
Copy link
Copy Markdown
Member

inteon commented Dec 12, 2023

I agree, combining GH with prow is no big deal.
Having said that, if someone wants to move this to make; I would approve that PR too.
/unhold

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 12, 2023
@maelvls
Copy link
Copy Markdown
Member

maelvls commented Dec 12, 2023

/approve

@jetstack-bot
Copy link
Copy Markdown
Contributor

jetstack-bot commented Dec 12, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 12, 2023
@jetstack-bot jetstack-bot merged commit 4ae2578 into cert-manager:master Dec 12, 2023
@wallrj wallrj deleted the golangci-lint branch December 12, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maelvls maelvls Awaiting requested review from maelvls

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wallrj @maelvls @inteon @jetstack-bot