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.

Sign up for GitHub

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

Jump to bottom

Add CI linter #699

Merged
merged 2 commits into from
Jan 28, 2019
Merged

Add CI linter #699

merged 2 commits into from
Jan 28, 2019

Conversation

vincepri
Copy link
Member

@vincepri vincepri commented Jan 19, 2019 •
edited
Loading

Signed-off-by: Vince Prignano [email protected]

What this PR does / why we need it:
This PR introduces golangci linter in CI and as Makefile lint and lint-full commands. The goal is to catch errors and general improvements during the ci-make step.

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 #43

Special notes for your reviewer:
NOTE: The PR doesn't introduce go-lint yet. I'm going to open a new PR given the amount of changes required.

Release note:

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 19, 2019
@vincepri vincepri force-pushed the add-ci-linter branch 2 times, most recently from 08d3f06 to 6f7188c Compare January 19, 2019 04:05
@vincepri
Copy link
Member Author

/test pull-cluster-api-test

2 similar comments
@vincepri
Copy link
Member Author

/test pull-cluster-api-test

@vincepri
Copy link
Member Author

/test pull-cluster-api-test

@neolit123
Copy link
Member

i see this test flaking a lot:

I0119 06:12:32.304] --- FAIL: TestReconcile (5.58s)
I0119 06:12:32.305] 	machinedeployment_controller_test.go:174: timed out waiting for value: 1
I0119 06:12:32.305] FAIL
I0119 06:12:32.305] FAIL	sigs.k8s.io/cluster-api/pkg/controller/machinedeployment	29.835s

timeouts and flakes would imply that this might not be suited as a "unit test"?

@vincepri
Copy link
Member Author

@neolit123 I proposed a fix in #702

@vincepri
Copy link
Member Author

/test pull-cluster-api-test

tariq1890
tariq1890 reviewed Jan 21, 2019
View reviewed changes
.golangci.yml
@@ -0,0 +1,21 @@
linters:
enable:
- govet
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add megacheck?

Copy link
Contributor

Choose a reason for hiding this comment

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

also goimports ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added goimports. About megacheck, it's now called staticcheck. A quick run of it seems to yield a good amount of tests to fix, should we do that in a different PR later on?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! And I don't mind taking that task up :)

paulfantom
paulfantom reviewed Jan 22, 2019
View reviewed changes
WORKSPACE Outdated Show resolved Hide resolved
@vincepri
Copy link
Member Author

/assign @roberthbailey

@vincepri
Copy link
Member Author

/retest

@roberthbailey
Copy link
Contributor

@vincepri - I was (selfishly) waiting to review until @paulfantom or @neolit123 gave an lgtm.

@vincepri vincepri mentioned this pull request Jan 23, 2019
Merged
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 Jan 25, 2019
@vincepri
Copy link
Member Author

@neolit123 ptal

@paulfantom
Copy link

/lgtm

@k8s-ci-robot
Copy link
Contributor

@paulfantom: changing LGTM is restricted to assignees, and only kubernetes-sigs/cluster-api repo collaborators may be assigned issues.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@vincepri
Copy link
Member Author

vincepri commented Jan 25, 2019 •
edited
Loading

/test pull-cluster-api-test

@vincepri
Copy link
Member Author

/test pull-cluster-api-make

@vincepri
Add CI linter
9e2b6cf 
Signed-off-by: Vince Prignano <[email protected]>
@vincepri
Copy link
Member Author

This should be good to go
/assign @roberthbailey

@neolit123
Copy link
Member

neolit123 commented Jan 28, 2019 •
edited
Loading

/lgtm

as per @roberthbailey 's comment:

@vincepri - I was (selfishly) waiting to review until @paulfantom or @neolit123 gave an lgtm.

thanks @vincepri

(edit: although the approve label was added because Vince became an approver... ).

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2019
@k8s-ci-robot k8s-ci-robot merged commit 86f4e2c into kubernetes-sigs:master Jan 28, 2019
@vincepri vincepri mentioned this pull request Feb 1, 2019
Merged
@paulfantom paulfantom mentioned this pull request Mar 6, 2019
Merged
@vincepri vincepri deleted the add-ci-linter branch July 26, 2019 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulfantom paulfantom paulfantom left review comments

@tariq1890 tariq1890 tariq1890 left review comments

@krisnova krisnova Awaiting requested review from krisnova

@roberthbailey roberthbailey Awaiting requested review from roberthbailey

Assignees

@neolit123 neolit123

@roberthbailey roberthbailey

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 "Looks good to me", 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.

Run more automated checks on cluster-api repo
6 participants
@vincepri @neolit123 @roberthbailey @k8s-ci-robot @paulfantom @tariq1890