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 additional checks to golangci-lint (#5567) #5687

Conversation

rm3l
Copy link
Member

@rm3l rm3l commented Apr 22, 2022

What type of PR is this:
/kind task

What does this PR do / why we need it:
This configures golangci-lint with the following additional checks:

  • depguard, to check whether packages imported are allowed or denied. For example, this allows to check that the deprecated github.com/pkg/errors is not introduced again.
  • revive (successor of the deprecated golint), configured with the following rules:
    • error-strings, to check common conventions around error messages. This PR also fixes errors reported by this rule.

Additional checks might be added later on as needed.

Which issue(s) this PR fixes:
Fixes #5567

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:
make validate and all tests should pass.

- revive is a drop-in replacement for the deprecated golint Linter
- revive will allow to check for error strings

Note: We are purposely not using the latest golangci-lint (1.45.0)
but the minimum version from which revive is available.
This is because the latest 1.45.0 reports additional linting issues
(from govet and staticcheck for example). And fixing those side errors
is outside of the scope of this issue.
Also some of those issues are already fixed in redhat-developer#5497 (update to Go 1.17).
More rules can be added later on if needed
Some rules are purposely ignored when the
error messages represent top-level errors that are
displayed to be displayed as is to end users
For some reason, those were not reported by revive's error-strings rule,
but only by GoLand inspection tool.
@openshift-ci openshift-ci bot added the kind/task Issue is actionable task label Apr 22, 2022
@netlify
Copy link

netlify bot commented Apr 22, 2022

Deploy Preview for odo-docusaurus-preview canceled.

Name Link
🔨 Latest commit 25d0a2a
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/62665d55970f3b0009c4efe2

@odo-robot
Copy link

odo-robot bot commented Apr 22, 2022

Unit Tests on commit 7820f79 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 22, 2022

Windows Tests (OCP) on commit 7820f79 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 22, 2022

Kubernetes Tests on commit 7820f79 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 22, 2022

OpenShift Tests on commit 7820f79 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 22, 2022

Validate Tests on commit 7820f79 finished successfully.
View logs: TXT HTML

@kadel
Copy link
Member

kadel commented Apr 22, 2022

/approve

@openshift-ci
Copy link

openshift-ci bot commented Apr 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Apr 22, 2022
@rm3l rm3l linked an issue Apr 22, 2022 that may be closed by this pull request
Also replace "fmt.Errorf" by "errors.New" when the error message is static
@sonarcloud
Copy link

sonarcloud bot commented Apr 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 25, 2022
@openshift-merge-robot openshift-merge-robot merged commit 06550eb into redhat-developer:main Apr 25, 2022
cdrage pushed a commit to cdrage/odo that referenced this pull request Aug 31, 2022
…t-developer#5687)

* Document golangci-lint configuration options

* Do not limit the number of issues reported per linter

* Add check preventing the use of the deprecated 'github.com/pkg/errors' package

* Upgrade golangci-lint to 1.37.0, so we can use 'revive' Linter

- revive is a drop-in replacement for the deprecated golint Linter
- revive will allow to check for error strings

Note: We are purposely not using the latest golangci-lint (1.45.0)
but the minimum version from which revive is available.
This is because the latest 1.45.0 reports additional linting issues
(from govet and staticcheck for example). And fixing those side errors
is outside of the scope of this issue.
Also some of those issues are already fixed in redhat-developer#5497 (update to Go 1.17).

* Configure revive to check for error strings

More rules can be added later on if needed

* Fix issues reported by revive's error-strings rule

Some rules are purposely ignored when the
error messages represent top-level errors that are
displayed to be displayed as is to end users

* Fix more error-strings issues

For some reason, those were not reported by revive's error-strings rule,
but only by GoLand inspection tool.

* Fix missing `revive:disable:error-strings` comment directive

Also replace "fmt.Errorf" by "errors.New" when the error message is static
@rm3l rm3l deleted the 5567-add-additional-checks-to-golangci-lint branch December 1, 2022 16:35
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. Required by Prow. kind/task Issue is actionable task lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error messages should never start capitalize or end with punctuation add additional checks to golangci-lint
5 participants