Skip to content

Conversation

@zhuangqh
Copy link
Contributor

@zhuangqh zhuangqh commented Dec 5, 2018

Signed-off-by: zhuangqh [email protected]

Ⅰ. Describe what this PR did

make PouchContainer compatible with multi host ip.

result returned by hostname -i is split by space, but scanner split tokens by lines by default.

Ⅱ. Does this pull request fix one issue?

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

    result returned by 'hostname -i' is split by space,
    but scanner split tokens by lines by default

Signed-off-by: zhuangqh <[email protected]>
@zhuangqh zhuangqh requested a review from rudyfly December 5, 2018 06:45
@pouchrobot pouchrobot added kind/bug This is bug report for project size/XS labels Dec 5, 2018
@codecov
Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #2536 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2536      +/-   ##
==========================================
- Coverage    69.2%   69.19%   -0.01%     
==========================================
  Files         278      278              
  Lines       18494    18495       +1     
==========================================
  Hits        12798    12798              
+ Misses       4243     4240       -3     
- Partials     1453     1457       +4
Flag Coverage Δ
#criv1alpha1test 31.28% <100%> (-0.05%) ⬇️
#criv1alpha2test 35.62% <100%> (+0.35%) ⬆️
#integrationtest 40.55% <100%> (+0.01%) ⬆️
#nodee2etest 32.56% <100%> (-0.22%) ⬇️
#unittest 26.9% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pkg/system/system.go 68.57% <100%> (+0.45%) ⬆️
ctrd/image.go 76.75% <0%> (-2.2%) ⬇️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
pkg/meta/store.go 67.44% <0%> (-1.56%) ⬇️
daemon/mgr/container_utils.go 85.44% <0%> (-1.27%) ⬇️
cri/v1alpha1/cri.go 60.59% <0%> (-0.34%) ⬇️
daemon/mgr/container.go 59.12% <0%> (-0.22%) ⬇️
cri/v1alpha2/cri_wrapper.go 63.6% <0%> (ø) ⬆️
cri/v1alpha2/cri.go 70.18% <0%> (+0.25%) ⬆️
ctrd/container.go 58.41% <0%> (+0.39%) ⬆️
... and 2 more

}

scanner := bufio.NewScanner(bytes.NewReader(output))
scanner.Split(bufio.ScanWords)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add some comments here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@zhuangqh PTAL

@rudyfly
Copy link
Collaborator

rudyfly commented Dec 6, 2018

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 6, 2018
@rudyfly rudyfly merged commit 755f40d into AliyunContainerService:master Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants