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

Switch to golangci-lint for the gocheck target #3

Merged
merged 2 commits into from
Aug 27, 2020

Conversation

arjunrn
Copy link
Contributor

@arjunrn arjunrn commented Aug 26, 2020

Replace all the linting tooling with golangci-lint which is a wrapper for the major linting tools. The changes also add a script which ensures that the linting tool is present on the path before doing the actual linting.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2020
@@ -0,0 +1,15 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

This is a cool idea. It would be neat to see as much of it as possible made part of _lib and just have the configuration (some kind of declarative list of dep/ver/loc) in the convention subdir in a file with a known name.

Copy link
Member

Choose a reason for hiding this comment

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

Gah, ignore this comment. I was thinking of this completely wrong. Your intent was (correctly) to run this as part of the make target, not to run it during update.

@@ -71,8 +71,8 @@ docker-push: push

.PHONY: gocheck
gocheck: ## Lint code
boilerplate/openshift/golang_osd_cluster_operator/golint.sh
${GOENV} go vet ./cmd/... ./pkg/...
boilerplate/openshift/golang_osd_cluster_operator/ensure.sh golangci-lint
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I missed this line before

boilerplate/openshift/golang_osd_cluster_operator/golint.sh
${GOENV} go vet ./cmd/... ./pkg/...
boilerplate/openshift/golang_osd_cluster_operator/ensure.sh golangci-lint
GOLANGCI_LINT_CACHE=/tmp/golanci-cache golangci-lint run -c boilerplate/openshift/golang_osd_cluster_operator/golangci.yml ./...
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
GOLANGCI_LINT_CACHE=/tmp/golanci-cache golangci-lint run -c boilerplate/openshift/golang_osd_cluster_operator/golangci.yml ./...
GOLANGCI_LINT_CACHE=/tmp/golangci-cache golangci-lint run -c boilerplate/openshift/golang_osd_cluster_operator/golangci.yml ./...

I'm curious how this env var is used by golangci-lint. If you don't set it, does it use a default? (I couldn't find documentation for it at a quick glance.)

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'm not sure why it resolves to /.cache as the default directory for caching but this issue summarizes what's going on.

@arjunrn arjunrn changed the title [WIP] Switch to golangci-lint for the gocheck target Switch to golangci-lint for the gocheck target Aug 26, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2020
Signed-off-by: Arjun Naik <anaik@redhat.com>
@2uasimojo
Copy link
Member

/lgtm

Do we want to wait to merge this until one or more of the following is in place?

  • At least one or two guinea pig repositories, using the old thing, to keep the initial bootstrap PR smaller
  • In-repo testing framework
  • Automated merge

If not, that's fine, just a thought.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2020
Signed-off-by: Arjun Naik <anaik@redhat.com>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2020
@2uasimojo
Copy link
Member

Thanks for the updates @arjunrn

/lgtm again

@2uasimojo
Copy link
Member

Apparently gh picks and chooses which directives are allowed to have stuff on the same line.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2020
@arjunrn arjunrn closed this Aug 27, 2020
@arjunrn arjunrn reopened this Aug 27, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, arjunrn

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2020
@openshift-merge-robot openshift-merge-robot merged commit 4286407 into master Aug 27, 2020
2uasimojo added a commit to 2uasimojo/boilerplate that referenced this pull request Sep 2, 2020
Update the README for the openshift/golang_osd_cluster_operator
convention to stop talking about golint.sh and start talking about
golangci-lint. This was missed in 2c63aeb (PR openshift#3).
@cblecker cblecker deleted the golangci-lint branch February 11, 2021 23:08
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.

4 participants