-
Notifications
You must be signed in to change notification settings - Fork 349
Conversation
Signed-off-by: Mike Brown <brownwm@us.ibm.com>
.tool/lint
Outdated
set -o pipefail | ||
|
||
for d in $(find . -type d -not -iwholename '*.git*' -a -not -iname '.tool' -a -not -iwholename '*vendor*'); do | ||
${GOPATH}/bin/gometalinter \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not work when GOPATH contains multiple entries.
I think it is ok to assume PATH is set to contain gometalinter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me.. done
Signed-off-by: Mike Brown <brownwm@us.ibm.com>
Signed-off-by: Mike Brown <brownwm@us.ibm.com>
How about using https://github.com/kubernetes/repo-infra/tree/master/verify which can avoid code duplication. |
@xlgao-zju ok.. took a look at it. Here are my thoughts:
Maybe after repo-infra has aged a bit? I opened an issue in repo-infra to track these questions. |
pkg/server/service.go
Outdated
@@ -21,7 +21,7 @@ import ( | |||
|
|||
"k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" | |||
|
|||
_ "github.com/containerd/containerd/api/services/content" | |||
_ "github.com/containerd/containerd/api/services/content" // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing context for TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context was described below.. none of the functions are implemented :-) "_" is being used to hold the reference from autocomplete deleting it until the reference is used below..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IOW "github.com/containerd/containerd/api/services/content" is the ToDo :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the TODO
above these imports with some context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
I'm not familiar with gometalinter. Does this mean that
That's how kubernetes repo is doing it, every source file has a boilerplate. Let's follow the same pattern in this repo. :) |
@Random-Liu yes, if vet is on for gometalinter it runs go vet. Ok will follow the same pattern. Will get to that this evening. |
.gitignore
Outdated
@@ -0,0 +1 @@ | |||
cri-containerd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we compile the binary into a output directory? Either bin
or _output
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
pkg/server/service.go
Outdated
@@ -21,7 +21,7 @@ import ( | |||
|
|||
"k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" | |||
|
|||
_ "github.com/containerd/containerd/api/services/content" | |||
_ "github.com/containerd/containerd/api/services/content" // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the TODO
above these imports with some context?
.tool/lint
Outdated
set -o nounset | ||
set -o pipefail | ||
|
||
for d in $(find . -type d -not -iwholename '*.git*' -a -not -iname '.tool' -a -not -iwholename '*vendor*'); do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use not
? If we only care about go files, I believe most of our go files will be in cmd
and pkg
.
How about find . -type d -a \( -iwholename './pkg*' -o -iwholename './cmd*' \)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reason just following the pattern from cri-o yours works just as well but would need to be updated if we add any code somewhere else...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theirs also need to be updated when we include new non-go directories, right?
The only golang package I could image we'll add is test
directory. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have druthers here.. will try yours out :-)
.tool/lint
Outdated
gometalinter \ | ||
--exclude='error return value not checked.*(Close|Log|Print).*\(errcheck\)$' \ | ||
--exclude='.*_test\.go:.*error return value not checked.*\(errcheck\)$' \ | ||
--exclude='duplicate of.*_test.go.*\(dupl\)$' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. For my education, what kind of error are we trying to exclude here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/mibk/dupl
gometalinter uses a tool called dupl ^
This filters the _test.go results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the document it's not clear to me what the tool is doing, and why we should exclude the failure here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking for possible duplicated code. Typically unit test cases have some duplicated code. We could remove it and see what it does when we add unit tests :-)
.tool/lint
Outdated
--exclude='error return value not checked.*(Close|Log|Print).*\(errcheck\)$' \ | ||
--exclude='.*_test\.go:.*error return value not checked.*\(errcheck\)$' \ | ||
--exclude='duplicate of.*_test.go.*\(dupl\)$' \ | ||
--exclude='vendor\/.*' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of error are we trying to exclude here? Just for my education.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to skip lint on vendor code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand, we should have skip vendor in while
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really really don't want vendor. But you're correct it probably does not need to be on both the loop filter and the --exclude filter since we are passing single files. I imagine a possibility for one of the lint tools to pull in an import and test it as well but that's a guess. I'd have to remove it to see for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw you removed this. So we don't need this?
Makefile
Outdated
@echo " * 'install' - Install binaries to system locations" | ||
@echo " * 'binaries' - Build cri-containerd" | ||
@echo " * 'clean' - Clean artifacts" | ||
@echo " * 'lint' - Execute the source code linter" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we replace this with a verify
command to do all the verification all together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
Makefile
Outdated
endif | ||
|
||
lint: check-gopath | ||
@echo "checking lint" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only echo information for lint
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reason will modify
Makefile
Outdated
$(PROJECT)/cmd/cri-containerd | ||
|
||
clean: | ||
find . -name \*~ -delete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my education, what file do we want to cleanup with these 2 commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just temp files (from md2man) that we don't have atm so can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
Makefile
Outdated
git-validation -v -run DCO,short-subject -range $(EPOCH_TEST_COMMIT)..HEAD | ||
endif | ||
|
||
.PHONY: install.tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why install.tools
is not in help
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not PHONY following commands .install.gitvalidation
etc. ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reason.. usually one should phony the commands where the target is not the output of the command. I picked this up from cri-o will update.
Makefile
Outdated
go get -u github.com/alecthomas/gometalinter | ||
gometalinter --install | ||
|
||
.install.md2man: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this later when we need it. We can file an issue for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kk
Have addressed all review comments. Will add boilerplate testing to 'make verify' in a future PR. |
Tested a few tasks locally with success. One nit: A build will get triggered with the default task via |
make binaries is the current target for that.. but that's only until we have an official package / distribution plan. Till then.. either make binaries or all is our build target.. All is the default make target thus why make by itself targets all then binaries... |
hack/lint.sh
Outdated
set -o pipefail | ||
|
||
for d in $(find . -type d -a \( -iwholename './pkg*' -o -iwholename './cmd*' \)); do | ||
echo ${d} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: either remove this echo
or add some context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when you run make verify it outputs the context of "checking lint" which is a little slow so I figured let it show what directories lint is checking. Other than checking lint what additional context are you thinking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean something like: "checking directory ${d} ...". :)
.tool/lint
Outdated
--exclude='error return value not checked.*(Close|Log|Print).*\(errcheck\)$' \ | ||
--exclude='.*_test\.go:.*error return value not checked.*\(errcheck\)$' \ | ||
--exclude='duplicate of.*_test.go.*\(dupl\)$' \ | ||
--exclude='vendor\/.*' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw you removed this. So we don't need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one nit
@mikebrow Will merge this after you address comments and squash commits. |
Signed-off-by: Mike Brown <brownwm@us.ibm.com>
…endor Update hcsshim vendor commit to 60b5fa7eea6f95295888d71b0621eb1c1291fb67
Adds an initial makefile. Addresses issue #3
Patterned after kubernetes-incubator/cri-o
Signed-off-by: Mike Brown brownwm@us.ibm.com