Skip to content
This repository was archived by the owner on Dec 20, 2024. It is now read-only.

Conversation

@SataQiu
Copy link
Member

@SataQiu SataQiu commented Aug 12, 2019

Ⅰ. Describe what this PR did

  1. add boilerplate check script (for go files)
  2. complete all detected missing boilerplate
  3. ignore generated files

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Ⅳ. Describe how to verify it

./hack/check.sh

Ⅴ. Special notes for reviews

Next Steps:

  1. add update-boilerplate script
  2. add this check into CircleCI

@pouchrobot
Copy link
Contributor

We found this is your first time to contribute to Dragonfly, @SataQiu
👏 We really appreciate it.
Just remind that you have read the contribution guide: https://github.com/dragonflyoss/Dragonfly/blob/master/CONTRIBUTING.md
If you didn't, you should do that first. If done, welcome again and please enjoy hacking! 🍻

@SataQiu SataQiu force-pushed the feature-add-boilerplate-check branch from 91bf1a0 to 0ea6869 Compare August 12, 2019 10:47
@SataQiu
Copy link
Member Author

SataQiu commented Aug 12, 2019

/assign @allencloud

@allencloud
Copy link
Contributor

That is so good. I like it very much. Thanks so much. I will review this asap. @SataQiu 👏


# boilerplate check
echo "CHECK: boilerpalte, check code boilerplate"
result=$(git ls-files | xargs go run ./hack/boilerplate/check-boilerplate.go)
Copy link
Contributor

Choose a reason for hiding this comment

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

Dragonfly (new-807) $ git ls-files | xargs go run ./hack/boilerplate/check-boilerplate.go
Dragonfly (new-807) $ echo $?
0
version (new-807) $ vi version/version.go
Dragonfly (new-807) $ git ls-files | xargs go run ./hack/boilerplate/check-boilerplate.go
error validating "version/version.go": the file is missing a boilerplate
exit status 1

It works with the check file.

While I am afraid that make check fails since there is no docker daemon installed on my machine. I think make file should be friendly with developer's local machine, like windows, mac and so on. And there is no need to depend on the docker engine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I am also curious whether the ./hack/check-docker.sh can be stripped out from make check.
But hack/check.sh is OK, right?

check:
	@echo "Begin to check code formats."
	./hack/check.sh
	@echo "Begin to check dockerd whether is startup"
	./hack/check-docker.sh
.PHONY: check

Copy link
Contributor

Choose a reason for hiding this comment

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

whether the ./hack/check-docker.sh can be stripped out from make check.

I think we should strip that from make check.

hack/hack.sh is OK.

@allencloud
Copy link
Contributor

allencloud commented Aug 13, 2019

LGTM, but I think we should make it work in the integration test of travis.

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Aug 13, 2019
@allencloud allencloud removed the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Aug 13, 2019
@SataQiu
Copy link
Member Author

SataQiu commented Aug 13, 2019

LGTM, but I think we should make it work in the integration test of travis.

@allencloud
IMO, travis only runs docker-build, do you mean to add some checks, like boilerplate-check in circleci?
How about do that in another PR? That will change circleci's behavior.

@allencloud
Copy link
Contributor

LGTM, I would like to merge this first. And then we can keep moving on to polish the check scripts to fit travisCI. Thanks for your work again. @SataQiu

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Aug 14, 2019
@allencloud allencloud merged commit 13322ca into dragonflyoss:master Aug 14, 2019
starnop pushed a commit to starnop/Dragonfly that referenced this pull request Nov 27, 2019
…ate-check

feature: add boilerplate check script
inoc603 pushed a commit to inoc603/Dragonfly that referenced this pull request Dec 23, 2019
…ate-check

feature: add boilerplate check script
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants