Skip to content
This repository was archived by the owner on Jul 30, 2021. It is now read-only.

✨ using golangci-lint instead of verify-xxx.sh #236

Merged
merged 1 commit into from
Sep 23, 2019
Merged

✨ using golangci-lint instead of verify-xxx.sh #236

merged 1 commit into from
Sep 23, 2019

Conversation

SataQiu
Copy link
Contributor

@SataQiu SataQiu commented Sep 18, 2019 •
edited
Loading

What this PR does / why we need it:
using golangci-lint instead of verify-xxx.sh

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #223

/cc @chuckha

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 18, 2019
@SataQiu
Copy link
Contributor Author

SataQiu commented Sep 18, 2019

/assign @chuckha @detiber

@chuckha chuckha changed the title feature: using golangci-lint instead of golint ✨ using golangci-lint instead of golint Sep 18, 2019
@chuckha
Copy link
Contributor

chuckha commented Sep 18, 2019

/approve

very nice!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, SataQiu

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2019
detiber
detiber reviewed Sep 20, 2019
View reviewed changes
Copy link
Contributor

@detiber detiber left a comment

Choose a reason for hiding this comment

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

@SataQiu thank you for catching that we were using a separate tool/script where we didn't need to, I think we can slim it down even further based on the feedback I left.

hack/verify-all.sh Outdated
failure $? "verify-golint.sh" "${out}"
cd "${REPO_PATH}"
fi

if [[ "${VERIFY_GOVET:-true}" == "true" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

The GOLANGCI_LINT section below should already run govet, so this can be removed (along with the hack/verify-govet.sh script.

hack/verify-golint.sh
@@ -18,34 +18,5 @@ set -o errexit
set -o nounset
set -o pipefail

# shellcheck source=/dev/null
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to remove this whole file, since the invocation of verify-golangci-lint should already invoke the linters we are requesting here.

hack/verify-all.sh Outdated
@@ -73,13 +73,6 @@ if [[ "${VERIFY_GOFMT:-true}" == "true" ]]; then
cd "${REPO_PATH}"
Copy link
Contributor

Choose a reason for hiding this comment

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

The GOLANGCI_LINT section below already runs gofmt, so the VERIFY_GOFMT section can be removed (along with the hack/verify-gofmt.sh script.

Copy link
Contributor

Choose a reason for hiding this comment

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

The GOLANGCI_LINT section below already runs misspell, so the VERIFY_SPELLING section can be removed (along with the hack/verify-spelling.sh script.

Copy link
Contributor

Choose a reason for hiding this comment

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

The GOLANGCI_LINT section below already runs whitespace, so the VERIFY_WHITESPACE section can be removed (along with the hack/verify-whitespace.sh script.

Makefile Outdated
@@ -101,6 +101,14 @@ lint: $(GOLANGCI_LINT) ## Lint quickly using `golangci-lint --fast=true`
lint-full: $(GOLANGCI_LINT) ## Lint thoroughly using `golangci-lint --fase=false`
$(GOLANGCI_LINT) run -v --fast=false

.PHONY: lint-focus
lint-focus: $(GOLANGCI_LINT) ## Lint using specified linter
Copy link
Contributor

Choose a reason for hiding this comment

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

Since hack/verify-all.sh is already calling one of the other make targets, this target shouldn't be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much @detiber
Actually, I've been thinking about doing this for a long time.
After doing this, I feel the code is much cleaner :)

@SataQiu SataQiu force-pushed the fix-golint-20190918 branch from 21ec341 to 1a4c4b1 Compare September 23, 2019 02:31
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 23, 2019
@SataQiu SataQiu force-pushed the fix-golint-20190918 branch from 1a4c4b1 to 2b45243 Compare September 23, 2019 02:32
@SataQiu SataQiu changed the title ✨ using golangci-lint instead of golint ✨ using golangci-lint instead of verify-xxx.sh Sep 23, 2019
@detiber
Copy link
Contributor

detiber commented Sep 23, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2019
@k8s-ci-robot k8s-ci-robot merged commit 7942be9 into kubernetes-retired:master Sep 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@detiber detiber detiber left review comments

@chuckha chuckha Awaiting requested review from chuckha

Assignees

@chuckha chuckha

@detiber detiber

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extract golint into a tool in hack/tools
4 participants
@SataQiu @chuckha @k8s-ci-robot @detiber