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

Add the golangci-lint GitHub action #6537

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Dec 7, 2023

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
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
Copy link
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
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
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
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
Member

maelvls commented Dec 12, 2023

/approve

@jetstack-bot
Copy link
Contributor

[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

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
7 checks passed
@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
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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants