Skip to content

Conversation

@rudyfly
Copy link
Collaborator

@rudyfly rudyfly commented Nov 26, 2018

Ⅰ. Describe what this PR did

add start lxcfs
get the root dir of pouch daemon, instead of /var/lib/pouch
modify ipv6 test for alikernel

Ⅱ. Does this pull request fix one issue?

NONE

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

Just modify test

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Signed-off-by: Rudy Zhang [email protected]

@codecov
Copy link

codecov bot commented Nov 26, 2018

Codecov Report

Merging #2504 into master will decrease coverage by 4.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2504      +/-   ##
==========================================
- Coverage   69.31%   65.06%   -4.25%     
==========================================
  Files         278      278              
  Lines       18387    18387              
==========================================
- Hits        12745    11964     -781     
- Misses       4213     5128     +915     
+ Partials     1429     1295     -134
Flag Coverage Δ
#criv1alpha1test 35.22% <ø> (+3.82%) ⬆️
#criv1alpha2test 35.49% <ø> (+0.03%) ⬆️
#integrationtest 40.46% <ø> (-0.07%) ⬇️
#nodee2etest 32.69% <ø> (-0.14%) ⬇️
#unittest 26.76% <ø> (ø) ⬆️
Impacted Files Coverage Δ
cri/v1alpha1/cri_types.go 0% <0%> (-100%) ⬇️
cri/v1alpha1/service/cri.go 0% <0%> (-84.62%) ⬇️
cri/v1alpha1/server.go 0% <0%> (-79.2%) ⬇️
cri/v1alpha1/cri.go 0% <0%> (-60.93%) ⬇️
cri/v1alpha1/cri_wrapper.go 0% <0%> (-55.31%) ⬇️
cri/v1alpha1/cri_utils.go 60.37% <0%> (-23.16%) ⬇️
cri/criservice.go 34.52% <0%> (-21.43%) ⬇️
apis/server/utils.go 71.15% <0%> (-3.85%) ⬇️
cri/v1alpha2/cri_wrapper.go 62.4% <0%> (-2.4%) ⬇️
ctrd/image.go 76.75% <0%> (-2.2%) ⬇️
... and 3 more

@pouchrobot pouchrobot added areas/test kind/bug This is bug report for project size/M labels Nov 26, 2018
func (suite *PouchInspectSuite) TestInspectCreateAndStartedFormat(c *check.C) {
name := "TestInspectCreateAndStartedFormat"
// get root dir
ret := command.PouchRun("info")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can wrap this logic to a function GetRootDir?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

get it

@HusterWan
Copy link
Contributor

please rebase your code

add start lxcfs
get the root dir of pouch daemon, instead of `/var/lib/pouch`
modify ipv6 test for alikernel

Signed-off-by: Rudy Zhang <[email protected]>
// Inspect LogPath, LogPath should not be empty after container's start.
// by default, the container has log type of json-file.
output = command.PouchRun("inspect", "-f", "{{.LogPath}}", name).Stdout()
expectedLogPath := fmt.Sprintf("/var/lib/pouch/containers/%s/json.log", containerID)
Copy link
Contributor

Choose a reason for hiding this comment

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

I search in pouch , find more hard code /var/lib/pouch, maybe we can fix in this pr to replace it use this func?

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 can't find anyone in testcase....Can you give a case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean in whole project can do as the same way, not only the testcase.

Copy link
Contributor

@zhuangqh zhuangqh Nov 28, 2018

Choose a reason for hiding this comment

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

I've searched the whole project. Actually, /var/lib/pouch occurred in 5 go file.
One of them is the default config of pouchd. Two of them is the default config of storage.

Two of them inside the test.
First one, a mock test, acceptable.
https://github.com/alibaba/pouch/blob/1f7dbf60d406210c65fb96ea4682f6d04f97773e/client/system_info_test.go#L40

Second one, a hard code path in cli test.
https://github.com/alibaba/pouch/blob/638b38fa9a09ae52922a900d611817b69e017d7c/test/cli_inspect_test.go#L71
I think this one could use GetRootDir function for more generality. Actually, this one was fixed in this PR.

@ZYecho

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM @zhuangqh thanks for your check.

@zhuangqh
Copy link
Contributor

LGTM

@zhuangqh zhuangqh merged commit ff90685 into AliyunContainerService:master Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

areas/test kind/bug This is bug report for project size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants