Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Update linters and gofmt script #1274

Conversation

jimmidyson
Copy link
Contributor

What this PR does / why we need it:
Update golangci-lint configuration to explicitly list all enabled linters and add a few more to the existing set. The original change was driven by a desire to remove gofmt scripts and relying on golangci-lint in combination with goimports to provide formatting and validation.

Second commit fixes all lint failures.

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 #

Special notes for your reviewer:
Sorry for PRs like this but as we're working on the codebase more, I would like to ensure we have as good tooling for keeping a consistent codebase as much as possible.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 17, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jimmidyson

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 17, 2020
@jimmidyson jimmidyson changed the title Jimmi/fix update gofmt script Update linters and gofmt script Aug 17, 2020
@jimmidyson
Copy link
Contributor Author

/cc @hectorj2f

@k8s-ci-robot k8s-ci-robot requested a review from hectorj2f August 17, 2020 13:39
@jimmidyson jimmidyson force-pushed the jimmi/fix-update-gofmt-script branch from e60fffb to 751997b Compare August 17, 2020 13:41
Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

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

LGTM, I think it is better using pre-commit to run the linters. Thanks

@hectorj2f
Copy link
Contributor

@irfanurrehman @RainbowMango do you have any comment regarding this change ? Otherwise I'd say, we can merge it.

@RainbowMango
Copy link
Contributor

Just quickly went through the PR. LGTM. Thanks.

Would you like to take an eye on it? @irfanurrehman

@RainbowMango
Copy link
Contributor

I can imagine it becomes harder to make the robot happy:) Hope people/contributor won't get crazy with so many linters :)
Maybe we should consider improve the performance of CI job later.

@hectorj2f
Copy link
Contributor

I'd wait for @irfanurrehman before giving it the #/lgtm

@jimmidyson
Copy link
Contributor Author

jimmidyson commented Aug 18, 2020

@RainbowMango the majority of these linters were already applied by virtue of being on by default. This pr just explicitly specifies then to reduce chance of surprise on upgrade of golangci-lint.

Makefile Outdated Show resolved Hide resolved
@irfanurrehman
Copy link
Contributor

Thanks for doing this @jimmidyson.
I have just one suggestion, pending which the PR looks good to me.

@jimmidyson jimmidyson force-pushed the jimmi/fix-update-gofmt-script branch from 751997b to 9aaf65a Compare August 19, 2020 08:01
@irfanurrehman
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2020
@k8s-ci-robot k8s-ci-robot merged commit 57a1852 into kubernetes-retired:master Aug 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants