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.

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

docs: user general pouchlinter to linter pouch's code #2421

Merged

Conversation

allencloud
Copy link
Collaborator

@allencloud allencloud commented Nov 4, 2018

Signed-off-by: Allen Sun [email protected]

Ⅰ. Describe what this PR did

user general pouchlinter to linter pouch's code.

For more info about pouchlinter, please check https://github.com/pouchcontainer/pouchlinter

The doc change in test.md and pouch_with_plugin.md is according to markdowinlint rule https://github.com/markdownlint/markdownlint/blob/master/docs/RULES.md#md046---code-block-style .

Ⅱ. Does this pull request fix one issue?

none

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

no need

Ⅳ. Describe how to verify it

none

Ⅴ. Special notes for reviews

none

@codecov
Copy link

codecov bot commented Nov 4, 2018

Codecov Report

Merging #2421 into master will increase coverage by 4.53%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2421      +/-   ##
==========================================
+ Coverage   64.07%   68.61%   +4.53%     
==========================================
  Files         275      275              
  Lines       18309    18309              
==========================================
+ Hits        11731    12562     +831     
+ Misses       5292     4327     -965     
- Partials     1286     1420     +134
Flag Coverage Δ
#criv1alpha1test 31.62% <ø> (?)
#criv1alpha2test 35.94% <ø> (+0.26%) ⬆️
#integrationtest 39.87% <ø> (-0.02%) ⬇️
#nodee2etest 33.3% <ø> (+0.14%) ⬆️
#unittest 25.49% <ø> (ø) ⬆️
Impacted Files Coverage Δ
daemon/mgr/container.go 60.66% <0%> (+0.63%) ⬆️
cri/v1alpha2/cri.go 68.53% <0%> (+0.83%) ⬆️
cri/v1alpha2/cri_wrapper.go 62.4% <0%> (+1.19%) ⬆️
ctrd/container.go 59.76% <0%> (+1.9%) ⬆️
ctrd/image.go 78.94% <0%> (+2.19%) ⬆️
cri/stream/runtime.go 70.58% <0%> (+2.35%) ⬆️
daemon/containerio/cri_log_file.go 88.23% <0%> (+3.92%) ⬆️
ctrd/watch.go 83.33% <0%> (+4.54%) ⬆️
cri/stream/portforward/httpstream.go 75.83% <0%> (+6.66%) ⬆️
cri/criservice.go 55.95% <0%> (+21.42%) ⬆️
... and 7 more

@allencloud allencloud force-pushed the use-pouchlinter branch 2 times, most recently from f98b70d to ddb1434 Compare November 4, 2018 15:26
@allencloud
Copy link
Collaborator Author

The gometalinter works. @fuweid
Do we need to remove the shell scripts in folder ./hack/...... ?
I am not sure if they are still useful?

@fuweid
Copy link
Contributor

fuweid commented Nov 5, 2018

Some scripts are used to help developer to install dependences easier. I think we can keep it.

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

command: make check
name: validate go code with gometalinter
command: |
gometalinter --disable-all --skip vendor -E gofmt -E goimports -E golint -E ineffassign -E misspell -E vet -E megacheck ./...
Copy link
Contributor

Choose a reason for hiding this comment

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

one more question, could we just use make check here, since the image provides the binary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think both are OK, while if we use make check, the calling path is a little bit long. For the test part and ci part, in my opinion, simplicity is the most important thing.

@allencloud allencloud merged commit 58d409d into AliyunContainerService:master Nov 6, 2018
@allencloud allencloud deleted the use-pouchlinter branch November 6, 2018 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants