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 fmt and vet make target #53

Merged
merged 1 commit into from
Dec 22, 2016
Merged

add fmt and vet make target #53

merged 1 commit into from
Dec 22, 2016

Conversation

andyxning
Copy link
Member

@andyxning andyxning commented Dec 21, 2016 •
edited by k8s-reviewable
Loading

Add fmt and vet make target. And, add fmt and vet dependencies for node-problem-detector target.

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 21, 2016
@andyxning andyxning mentioned this pull request Dec 21, 2016
Merged
@andyxning
Copy link
Member Author

@Random-Liu PTAL.

@Random-Liu
Copy link
Member

@andyxning Seems like something wrong with the rebase. :)

@andyxning andyxning force-pushed the add_fmt_and_vet_make_target branch from 8e03f63 to 51609be Compare December 21, 2016 09:34
@Random-Liu
Copy link
Member

@andyxning There are still 2 duplicated commits. :)

@andyxning andyxning force-pushed the add_fmt_and_vet_make_target branch from 51609be to 0e2e5c0 Compare December 21, 2016 09:55
@andyxning
Copy link
Member Author

@Random-Liu Done Again. :)

Random-Liu
Random-Liu reviewed Dec 21, 2016
View reviewed changes
Makefile
@@ -9,7 +9,13 @@ PROJ = google_containers

PKG_SOURCES := $(shell find pkg -name '*.go')

node-problem-detector: $(PKG_SOURCES) node_problem_detector.go
vet:
go list ./... | grep -v "./vendor*" | xargs go vet
Copy link
Member

Choose a reason for hiding this comment

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

This seems a little weird to me although it works for now.

go list is listing packages, while grep -v "./vendor*" seems to assume that it is listing files.
In the future, if there is a package called "abc/vendorefg", it will also be excluded.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Random-Liu Just like next comment, we can change grep -v "./vendor*" to grep -v "./vendor/*" to minimize the impact.

Copy link
Member

@Random-Liu Random-Liu Dec 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Just to make sure, do you expect . to match a dot or matching anything?

Copy link
Member Author

@andyxning andyxning Dec 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

What . means is that it means anything prefixing vendor directory. Because we run go list ./... in the root directory, it output something like this

k8s.io/node-problem-detector
k8s.io/node-problem-detector/pkg/condition
k8s.io/node-problem-detector/pkg/kernelmonitor
k8s.io/node-problem-detector/pkg/kernelmonitor/translator
k8s.io/node-problem-detector/pkg/kernelmonitor/types
k8s.io/node-problem-detector/pkg/kernelmonitor/util
k8s.io/node-problem-detector/pkg/problemclient
k8s.io/node-problem-detector/pkg/problemdetector
k8s.io/node-problem-detector/pkg/types
k8s.io/node-problem-detector/pkg/util
k8s.io/node-problem-detector/pkg/util/nethealth
k8s.io/node-problem-detector/vendor/code.cloudfoundry.org/clock
k8s.io/node-problem-detector/vendor/code.cloudfoundry.org/clock/fakeclock
k8s.io/node-problem-detector/vendor/github.com/blang/semver
k8s.io/node-problem-detector/vendor/github.com/coreos/go-oidc/http

What i mean is to match.*/vendor/.* in Golang regexp expression.

Copy link
Member

Choose a reason for hiding this comment

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

@andyxning Yeah, I think you should be assuming to match everything.

Makefile
go list ./... | grep -v "./vendor*" | xargs go vet

fmt:
find . -type f -name "*.go" | grep -v "./vendor*" | xargs gofmt -s -w
Copy link
Member

Choose a reason for hiding this comment

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

How about `grep -v "./vendor/*" to make it less ambiguous.

Copy link
Member

Choose a reason for hiding this comment

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

How about -l to also list bad files?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Random-Liu Agree to add -l to list bad files. Done.

@andyxning andyxning force-pushed the add_fmt_and_vet_make_target branch from 0e2e5c0 to bb48848 Compare December 21, 2016 11:20
@Random-Liu
Copy link
Member

LGTM. And could you help me review this #56?

@Random-Liu Random-Liu merged commit 750c292 into kubernetes:master Dec 22, 2016
@Random-Liu Random-Liu added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 22, 2016
@andyxning andyxning deleted the add_fmt_and_vet_make_target branch January 18, 2017 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Random-Liu Random-Liu Random-Liu left review comments

Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andyxning @Random-Liu @k8s-ci-robot