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

Add verify-gomod and verify-goimports to Travis job #7952

Merged
merged 3 commits into from
Nov 27, 2019

Conversation

rifelpet
Copy link
Member

It turns out we werent running verify-goimports in any CI job and some files were failing. This adds them in a blocking job which should prevent that from happening again.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 19, 2019
@rifelpet rifelpet mentioned this pull request Nov 19, 2019
@rifelpet
Copy link
Member Author

I'll probably need to vendor goimports the same way we do staticcheck and a few other tools in tools.go

@rifelpet rifelpet changed the title Add verify-gomod and verify-goimports to Travis job [WIP] Add verify-gomod and verify-goimports to Travis job Nov 19, 2019
@k8s-ci-robot k8s-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 Nov 19, 2019
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 19, 2019
@rifelpet rifelpet changed the title [WIP] Add verify-gomod and verify-goimports to Travis job Add verify-gomod and verify-goimports to Travis job Nov 19, 2019
@k8s-ci-robot k8s-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 Nov 19, 2019
@rifelpet
Copy link
Member Author

Travis is passing and definitely running verify-goimports (see the failing job here) so I think this is ready for review 👍

@geojaz
Copy link
Member

geojaz commented Nov 25, 2019

@rifelpet Any reason why this should go into travis and not prow? I noticed that we weren't tracking this test while reviewing a patch that was green on prow, but not locally (with make ci). I'd love to see this in prow if possible.

@rifelpet
Copy link
Member Author

No real reason for adding it to Travis vs Prow. We could easily add new prow jobs to test-infra that run each of these two commands, or perhaps collapse all of the verify-* commands into one prow job.

Thoughts?

@geojaz
Copy link
Member

geojaz commented Nov 25, 2019

I think it probably makes sense to collapse the "linty" stuff verify-* into a single prow job. i will defer to some others who are more recently involved in this space...

@johngmyers
Copy link
Member

johngmyers commented Nov 25, 2019

The advantage of putting it in Travis is that the problems can be detected and fixed before the ok-to-test label is applied. That would save the reviewers some rounds of re-review.

@geojaz
Copy link
Member

geojaz commented Nov 26, 2019

The advantage of putting it in Travis is that the problems can be detected and fixed before the ok-to-test label is applied. That would save the reviewers some rounds of re-review.

Good point. @rifelpet can you update this and we can get it included?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet

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 Nov 27, 2019
@rifelpet
Copy link
Member Author

@geojaz done!

Copy link
Member

@geojaz geojaz left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 27, 2019
@k8s-ci-robot k8s-ci-robot merged commit 63ec879 into kubernetes:master Nov 27, 2019
@rifelpet rifelpet deleted the travis-verify branch August 6, 2020 02:51
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. 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants