Improve code checking #131

phoracek · 2019-06-02T12:16:42Z

Let's automate more code validation. And reorganize Makefile targets a little.

phoracek · 2019-06-02T12:20:13Z

@booxter @SchSeba @qinqon please take a look

SchSeba

overall looks good just two small nits

.travis.yml

SchSeba

LGTM!

qinqon

To be sure to run static code checks and code generations tools before calling docker build we can track source files with $(wildcard..) and generate little dummy files with the target name with touch $@ this way we ensure that we call docker build with perfect code and without taking much time at noop.

Makefile

qinqon

LGTM, we have to be careful with deletes though.

Makefile

Signed-off-by: Petr Horacek <phoracek@redhat.com>

booxter

my comments are largely nits / nice to haves so not blocking the merge here.

README.md

hack/verify-codegen.sh

hack/whitespace.sh

qinqon

Found how to fix rebuild on deletes, let's just add that and merge.

Makefile

phoracek · 2019-06-04T17:48:25Z

@qinqon sorry man, but I also need all_sources (exluding vendor/), and I'm not able to pull that off using beautiful Makefile tools. Tried following, but it doesn't filter-out anything:

vendor_sources := $(call rwildcard,vendor/,*)
all_sources := $(filter-out $(vendor_sources), $(call rwildcard,.)) .

We can reiterate on that part.

phoracek · 2019-06-04T17:54:40Z

@booxter @qinqon thanks a lot for the previous review. Should be fixed, could you give it another iteration?

qinqon

Finding all files is loaded everytime we call make... maybe is time to move to pre-commit.

Also you have to replace find by the rwildcard

qinqon · 2019-06-05T05:32:15Z

Makefile

+
+# Gather needed source files and directories to create target dependencies
+all_sources := $(shell git ls-files -- ':!vendor/')
+cmd_sources := $(shell find ./cmd/ -type f -name '*.go')


You have to use the rwildcard make function we just created and also add the directory to detect deletes.

# Gather needed source files and directories to create target dependencies cmd_sources := $(call rwildcard,cmd/,*.go) cmd/ pkg_sources := $(call rwildcard,pkg/,*.go) pkg/ apis_sources := $(call rwildcard,pkg/apis/,*.go) pkg/apis/

#131 (comment)

qinqon · 2019-06-05T05:39:08Z

Makefile

+pkg_sources := $(shell find ./pkg/ -type f -name '*.go')
+apis_sources := $(shell find ./pkg/apis/ -type f -name '*.go')
+
+petr:


Have to be good to be a make target :-)

Petr deserves it

qinqon · 2019-06-05T05:44:31Z

Makefile

+rwildcard=$(wildcard $1$2) $(foreach d,$(wildcard $1*),$(call rwildcard,$d/,$2))
+
+# Gather needed source files and directories to create target dependencies
+all_sources := $(shell git ls-files -- ':!vendor/')


We have to be careful making this depend on git, if we use at build we are not going to be able to build from tarball for example.

This sanity check stuff is starting to blow out of proportion, at openstack we use pre-commit and there some pre-commit hooks for golang

make whitespace-check 0.09s user 0.02s system 97% cpu 0.114 total

Use goimports to check code: goimports checks what gofmt does plus it keeps imports in specific order. In this patch we: - replace gofmt with goimports - add code format checking to travis - reorganize Makefile a little bit - remove make generate-manifests from README.md since it is not needed during the development process - add Makefile target for operator-sdk build k8s Add whitespace check and auto-formatting: I hate trailing whitespaces. This patch adds automated check detecting any and Makefile target removing them. Add codegen validation: Add a check validating that generated code is up to date. This would fail on travis in case a contributor forget to run `make generate-api`. ship and goimports operator-sdk in vendor/: In order to make sure everyone uses the same version of operator-sdk producing the same code, let's keep operator-sdk in vendoring. The same for goimports. Signed-off-by: Petr Horacek <phoracek@redhat.com>

phoracek · 2019-06-05T15:05:22Z

ci test please

booxter

I love approving PRs with 1.5 mln LOC.

phoracek · 2019-06-05T19:07:02Z

Feels good, thanks guys

kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L labels Jun 2, 2019

phoracek force-pushed the improve_code_checking branch from f62f265 to 6c9fc87 Compare June 2, 2019 12:17

phoracek changed the title ~~WIP Improve code checking~~ Improve code checking Jun 2, 2019

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 2, 2019

phoracek requested review from SchSeba, booxter and qinqon June 2, 2019 12:19

phoracek force-pushed the improve_code_checking branch from 3fa39f7 to fbdcdc5 Compare June 2, 2019 12:38

phoracek self-assigned this Jun 2, 2019

phoracek force-pushed the improve_code_checking branch 3 times, most recently from 22fdf59 to 37b5e21 Compare June 2, 2019 13:11

SchSeba suggested changes Jun 3, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Show resolved Hide resolved

phoracek force-pushed the improve_code_checking branch from 37b5e21 to 6763c47 Compare June 3, 2019 10:01

SchSeba previously approved these changes Jun 3, 2019

View reviewed changes

qinqon requested changes Jun 3, 2019

View reviewed changes

phoracek dismissed SchSeba’s stale review via c4d88c4 June 3, 2019 12:47

phoracek force-pushed the improve_code_checking branch 2 times, most recently from c4d88c4 to c632efe Compare June 3, 2019 15:36

kubevirt-bot added size/XXL and removed size/L labels Jun 3, 2019

phoracek force-pushed the improve_code_checking branch 3 times, most recently from 8c2db6e to 0203acf Compare June 3, 2019 15:54

qinqon previously approved these changes Jun 3, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

phoracek dismissed qinqon’s stale review via fe7bb97 June 3, 2019 16:06

phoracek force-pushed the improve_code_checking branch 2 times, most recently from fe7bb97 to de041b2 Compare June 3, 2019 16:07

phoracek force-pushed the improve_code_checking branch from de041b2 to f44d913 Compare June 3, 2019 16:22

add vendoring for improved checking

fb91be6

Signed-off-by: Petr Horacek <phoracek@redhat.com>

phoracek force-pushed the improve_code_checking branch 5 times, most recently from db0341a to d33880f Compare June 3, 2019 17:15

booxter previously approved these changes Jun 4, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

hack/verify-codegen.sh Outdated Show resolved Hide resolved

hack/whitespace.sh Outdated Show resolved Hide resolved

qinqon requested changes Jun 4, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

phoracek dismissed booxter’s stale review via 6690002 June 4, 2019 17:53

phoracek force-pushed the improve_code_checking branch from d33880f to 6690002 Compare June 4, 2019 17:53

phoracek force-pushed the improve_code_checking branch from 6690002 to 9df0636 Compare June 4, 2019 23:36

phoracek mentioned this pull request Jun 4, 2019

Functional tests #118

Merged

booxter previously approved these changes Jun 5, 2019

View reviewed changes

qinqon requested changes Jun 5, 2019

View reviewed changes

phoracek dismissed booxter’s stale review via 793e629 June 5, 2019 10:55

phoracek force-pushed the improve_code_checking branch 2 times, most recently from 793e629 to ef922a0 Compare June 5, 2019 11:42

qinqon previously approved these changes Jun 5, 2019

View reviewed changes

phoracek dismissed qinqon’s stale review via a6a1107 June 5, 2019 12:20

phoracek force-pushed the improve_code_checking branch from ef922a0 to a6a1107 Compare June 5, 2019 12:20

Merge branch 'master' into improve_code_checking

bc03c77

booxter approved these changes Jun 5, 2019

View reviewed changes

phoracek merged commit e0d21c5 into kubevirt:master Jun 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve code checking #131

Improve code checking #131

phoracek commented Jun 2, 2019 •

edited

phoracek commented Jun 2, 2019

SchSeba left a comment

SchSeba left a comment

qinqon left a comment

qinqon left a comment

booxter left a comment

qinqon left a comment

phoracek commented Jun 4, 2019

phoracek commented Jun 4, 2019

qinqon left a comment

qinqon Jun 5, 2019

phoracek Jun 5, 2019

qinqon Jun 5, 2019

phoracek Jun 5, 2019

booxter Jun 5, 2019

qinqon Jun 5, 2019

phoracek Jun 5, 2019

phoracek commented Jun 5, 2019

booxter left a comment

phoracek commented Jun 5, 2019

Improve code checking #131

Improve code checking #131

Conversation

phoracek commented Jun 2, 2019 • edited

phoracek commented Jun 2, 2019

SchSeba left a comment

Choose a reason for hiding this comment

SchSeba left a comment

Choose a reason for hiding this comment

qinqon left a comment

Choose a reason for hiding this comment

qinqon left a comment

Choose a reason for hiding this comment

booxter left a comment

Choose a reason for hiding this comment

qinqon left a comment

Choose a reason for hiding this comment

phoracek commented Jun 4, 2019

phoracek commented Jun 4, 2019

qinqon left a comment

Choose a reason for hiding this comment

qinqon Jun 5, 2019

Choose a reason for hiding this comment

phoracek Jun 5, 2019

Choose a reason for hiding this comment

qinqon Jun 5, 2019

Choose a reason for hiding this comment

phoracek Jun 5, 2019

Choose a reason for hiding this comment

booxter Jun 5, 2019

Choose a reason for hiding this comment

qinqon Jun 5, 2019

Choose a reason for hiding this comment

phoracek Jun 5, 2019

Choose a reason for hiding this comment

phoracek commented Jun 5, 2019

booxter left a comment

Choose a reason for hiding this comment

phoracek commented Jun 5, 2019

phoracek commented Jun 2, 2019 •

edited