initial makefile by mikebrow · Pull Request #7 · containerd/cri

mikebrow · 2017-04-15T15:00:00Z

Adds an initial makefile. Addresses issue #3

Patterned after kubernetes-incubator/cri-o

Signed-off-by: Mike Brown brownwm@us.ibm.com

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

AkihiroSuda · 2017-04-15T15:41:27Z

.tool/lint

+set -o pipefail
+
+for d in $(find . -type d -not -iwholename '*.git*' -a -not -iname '.tool' -a -not -iwholename '*vendor*'); do
+	${GOPATH}/bin/gometalinter \


This does not work when GOPATH contains multiple entries.

I think it is ok to assume PATH is set to contain gometalinter

works for me.. done

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

xlgao-zju · 2017-04-17T06:51:15Z

How about using https://github.com/kubernetes/repo-infra/tree/master/verify which can avoid code duplication.

mikebrow · 2017-04-17T13:43:51Z

@xlgao-zju ok.. took a look at it. Here are my thoughts:

I prefer the options selected for gometaliner in this PR over the ones in https://github.com/kubernetes/repo-infra/blob/master/verify/go-tools/verify-gometalinter.sh.
Not sure why we should run gometalinter with vet enabled and govet...
I see the need for boilerplate checking but not sure if we should be running that on every file. For example adding a copyright to a gofmt.sh that is verbatim from the examples might be overkill.

Maybe after repo-infra has aged a bit?

I opened an issue in repo-infra to track these questions.

cmluciano · 2017-04-17T15:12:14Z

pkg/server/service.go

 	"k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"

-	_ "github.com/containerd/containerd/api/services/content"
+	_ "github.com/containerd/containerd/api/services/content" // TODO


Missing context for TODO

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..

IOW "github.com/containerd/containerd/api/services/content" is the ToDo :)

Can we add the TODO above these imports with some context?

Random-Liu · 2017-04-17T18:08:00Z

Not sure why we should run gometalinter with vet enabled and govet...

I'm not familiar with gometalinter. Does this mean that go vet will run by gometalinter automatically? https://github.com/alecthomas/gometalinter#supported-linters

I see the need for boilerplate checking but not sure if we should be running that on every file. For example adding a copyright to a gofmt.sh that is verbatim from the examples might be overkill.

That's how kubernetes repo is doing it, every source file has a boilerplate. Let's follow the same pattern in this repo. :)

mikebrow · 2017-04-17T18:30:38Z

@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.

Random-Liu · 2017-04-17T17:47:50Z

.gitignore

@@ -0,0 +1 @@
+cri-containerd


Can we compile the binary into a output directory? Either bin or _output.

Random-Liu · 2017-04-17T17:48:47Z

pkg/server/service.go

 	"k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"

-	_ "github.com/containerd/containerd/api/services/content"
+	_ "github.com/containerd/containerd/api/services/content" // TODO


Can we add the TODO above these imports with some context?

Random-Liu · 2017-04-17T18:23:57Z

.tool/lint

+set -o nounset
+set -o pipefail
+
+for d in $(find . -type d -not -iwholename '*.git*' -a -not -iname '.tool' -a -not -iwholename '*vendor*'); do


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*' $?

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...

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?

Don't have druthers here.. will try yours out :-)

Random-Liu · 2017-04-17T18:28:57Z

.tool/lint

+	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\)$' \


Same here. For my education, what kind of error are we trying to exclude here?

https://github.com/mibk/dupl
gometalinter uses a tool called dupl ^
This filters the _test.go results.

From the document it's not clear to me what the tool is doing, and why we should exclude the failure here...

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 :-)

Random-Liu · 2017-04-17T18:28:58Z

.tool/lint

+		 --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\/.*' \


What kind of error are we trying to exclude here? Just for my education.

This is to skip lint on vendor code.

I don't understand, we should have skip vendor in while, right?

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.

I saw you removed this. So we don't need this?

Random-Liu · 2017-04-17T18:53:58Z

Makefile

+	@echo " * 'install' - Install binaries to system locations"
+	@echo " * 'binaries' - Build cri-containerd"
+	@echo " * 'clean' - Clean artifacts"
+	@echo " * 'lint' - Execute the source code linter"


Can we replace this with a verify command to do all the verification all together?

Random-Liu · 2017-04-17T18:55:52Z

Makefile

+endif
+
+lint: check-gopath
+	@echo "checking lint"


Why only echo information for lint?

no reason will modify

Random-Liu · 2017-04-17T18:57:48Z

Makefile

+		$(PROJECT)/cmd/cri-containerd
+
+clean:
+	find . -name \*~ -delete


For my education, what file do we want to cleanup with these 2 commands?

just temp files (from md2man) that we don't have atm so can be removed

Random-Liu · 2017-04-17T19:00:12Z

Makefile

+	git-validation -v -run DCO,short-subject -range $(EPOCH_TEST_COMMIT)..HEAD
+endif
+
+.PHONY: install.tools


Why install.tools is not in help?

Why not PHONY following commands .install.gitvalidation etc. ?

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.

Random-Liu · 2017-04-17T19:01:35Z

Makefile

+	go get -u github.com/alecthomas/gometalinter
+	gometalinter --install
+
+.install.md2man:


Add this later when we need it. We can file an issue for this.

mikebrow · 2017-04-18T00:59:04Z

Have addressed all review comments. Will add boilerplate testing to 'make verify' in a future PR.

cmluciano · 2017-04-18T17:47:47Z

Tested a few tasks locally with success.

One nit: A build will get triggered with the default task via make. It would be great to have an explicit build task so that make build works.

mikebrow · 2017-04-18T21:13:57Z

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...

Random-Liu · 2017-04-18T21:39:15Z

hack/lint.sh

+set -o pipefail
+
+for d in $(find . -type d -a \( -iwholename './pkg*' -o -iwholename './cmd*' \)); do
+	echo ${d}


nit: either remove this echo or add some context.

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?

I mean something like: "checking directory ${d} ...". :)

Random-Liu · 2017-04-18T21:42:13Z

.tool/lint

+		 --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\/.*' \


I saw you removed this. So we don't need this?

Random-Liu

LGTM with one nit

Random-Liu · 2017-04-19T01:21:12Z

@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 initial makefile

b7abeb6

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

k8s-ci-robot added the cncf-cla: yes label Apr 15, 2017

mikebrow requested a review from Random-Liu April 15, 2017 15:01

AkihiroSuda reviewed Apr 15, 2017

View reviewed changes

mikebrow added 2 commits April 15, 2017 10:52

clean up lint

55ad9a6

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

presume path is set to contain gomealinter

4ac74ab

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

cmluciano reviewed Apr 17, 2017

View reviewed changes

Random-Liu reviewed Apr 17, 2017

View reviewed changes

Random-Liu self-assigned this Apr 18, 2017

Random-Liu reviewed Apr 18, 2017

View reviewed changes

cmluciano mentioned this pull request Apr 18, 2017

Add version flag #10

Merged

addresses requested improvements

1894a34

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

mikebrow merged commit 11ba1cb into containerd:master Apr 19, 2017

This was referenced Apr 19, 2017

Create a Makefile #3

Closed

boilerplate verification is needed. #11

Closed

dcantah pushed a commit to dcantah/cri that referenced this pull request Jan 7, 2022

Merge pull request containerd#7 from katiewasnothere/update_hcsshim_v…

1773b94

…endor Update hcsshim vendor commit to 60b5fa7eea6f95295888d71b0621eb1c1291fb67

		@@ -0,0 +1 @@
		cri-containerd

Conversation

mikebrow commented Apr 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xlgao-zju commented Apr 17, 2017

Uh oh!

mikebrow commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikebrow commented Apr 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow commented Apr 15, 2017 •

edited

Loading

mikebrow commented Apr 17, 2017 •

edited

Loading

mikebrow Apr 17, 2017 •

edited

Loading

Random-Liu commented Apr 17, 2017 •

edited

Loading

Random-Liu Apr 17, 2017 •

edited

Loading

mikebrow Apr 17, 2017 •

edited

Loading

mikebrow Apr 17, 2017 •

edited

Loading