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

Integrate golangci-lint to run aggregate linting checks for CI #2536

Conversation

timflannagan
Copy link
Contributor

Description of the change:

Motivation for the change:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive

@timflannagan
Copy link
Contributor Author

/wip

Introduce a root .golangci-lint.yaml file to the repository. This file
is responsible for housing the golangci-lint configuration, and the
various aggregate linters that are enabled/disabled.

Signed-off-by: timflannagan <timflannagan@gmail.com>
Update the sanity workflow and add the golangci-lint action.

Signed-off-by: timflannagan <timflannagan@gmail.com>
- unconvert
- whitespace
disable:
- errcheck
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this can be done in a follow-up PR.

Comment on lines +25 to +26
max-issues-per-linter: 0
max-same-issues: 0
Copy link
Contributor Author

@timflannagan timflannagan Dec 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting both of these configuration options limit the number of linting violations that are discovered at a time. Setting the values to 0 disables that behavior.

@@ -69,7 +69,7 @@ var (
tlsCertPath = flag.String(
"tls-cert", "", "Path to use for certificate key (requires tls-key)")

profiling = flag.Bool("profiling", false, "deprecated")
_ = flag.Bool("profiling", false, "deprecated")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag is currently unused right now since we merge the profiling/metrics/health/etc. endpoints last minor release. I thought this was the easiest solution, vs. updating the .golangci-lint.yaml configuration and explicitly allowlisting this flag.

Comment on lines +1715 to +1718
if err != nil {
logger.WithError(err).Warn("failed to list operatorgroups")
return
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: need to double-check whether this is the correct behavior. It wasn't clear to me poking around whether I should be returning a syncError here.

- depguard
- gofmt
- goimports
- importas
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to tackle adding a more fleshed out importas configuration ( e.g. enforcing consistency with corev1/metav1/operatorsv1alpha1/etc. package alias') as a follow-up.

Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Just a quick question.

Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 22, 2021
Signed-off-by: timflannagan <timflannagan@gmail.com>
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

Great work on this @timflannagan

@openshift-ci
Copy link

openshift-ci bot commented Dec 23, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, dinhxuanvu, timflannagan

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:
  • OWNERS [awgreene,dinhxuanvu]

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

@timflannagan timflannagan added the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2021
@openshift-merge-robot openshift-merge-robot merged commit b3cded1 into operator-framework:master Dec 23, 2021
@timflannagan timflannagan deleted the ci/golangci-lint-integration branch December 23, 2021 19:45
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants