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

feature: add volume ls quiet flag support #2464

Merged
merged 1 commit into from
Nov 22, 2018

Conversation

ZYecho
Copy link
Contributor

@ZYecho ZYecho commented Nov 11, 2018

Signed-off-by: zhangyue [email protected]

Ⅰ. Describe what this PR did

add pouch volume list --quiet flag to support only show volume name.

Ⅱ. Does this pull request fix one issue?

fix #2462 feature request.

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

Add integration test case.

Ⅳ. Describe how to verify it

pouch volume ls --quiet

Ⅴ. Special notes for reviews

None.

@codecov
Copy link

codecov bot commented Nov 11, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2464      +/-   ##
==========================================
+ Coverage   68.76%   68.95%   +0.19%     
==========================================
  Files         277      277              
  Lines       18330    18330              
==========================================
+ Hits        12604    12640      +36     
+ Misses       4291     4265      -26     
+ Partials     1435     1425      -10
Flag Coverage Δ
#criv1alpha1test 31.63% <ø> (+0.02%) ⬆️
#criv1alpha2test 35.54% <ø> (-0.24%) ⬇️
#integrationtest 40.36% <ø> (+0.09%) ⬆️
#nodee2etest 33.08% <ø> (+0.18%) ⬆️
#unittest 26.69% <ø> (ø) ⬆️
Impacted Files Coverage Δ
cri/v1alpha2/cri.go 70.03% <0%> (-0.13%) ⬇️
cri/v1alpha2/cri_wrapper.go 63.6% <0%> (ø) ⬆️
daemon/mgr/container.go 59% <0%> (+0.64%) ⬆️
ctrd/container.go 58.89% <0%> (+0.79%) ⬆️
daemon/mgr/spec_linux.go 79.32% <0%> (+1.01%) ⬆️
daemon/logger/jsonfile/utils.go 73.17% <0%> (+1.62%) ⬆️
ctrd/image.go 78.94% <0%> (+2.19%) ⬆️
ctrd/client.go 69.23% <0%> (+2.3%) ⬆️
pkg/streams/utils.go 91.66% <0%> (+2.38%) ⬆️
daemon/mgr/events.go 100% <0%> (+3.7%) ⬆️
... and 2 more

ctx := context.Background()
volumes, err := apiClient.VolumeList(ctx)
if err != nil {
return errors.Wrap(err, "fali to list volumes")
Copy link
Contributor

Choose a reason for hiding this comment

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

just one typo: fali

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, update.

cli/volume.go Show resolved Hide resolved
cli/volume.go Outdated Show resolved Hide resolved
c.Assert(len(fields), check.Equals, 1)

for _, line := range lines {
if strings.Contains(line, volumeName) {
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 decrease the indent, WDYT?

@fuweid
Copy link
Contributor

fuweid commented Nov 21, 2018

we got flaky test or bug here

=== RUN   TestKMutexTimeout
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

@allencloud
Copy link
Collaborator

we got flaky test or bug here

You could add an issue for this. @fuweid

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.

Basically, LGTM

I think we can do better in test case since the check is not effective with strings.Contain

@ZYecho
Copy link
Contributor Author

ZYecho commented Nov 22, 2018

in the test case, just use strings.Contain to skip first line of output, but use equal to do extractMatch, I think it's enough here. @fuweid

@fuweid
Copy link
Contributor

fuweid commented Nov 22, 2018

in the test case, just use strings.Contain to skip first line of ouput, but use equal to do extractMatch, I think it's enough here.

I did one thing to parse the table into map[string][]string for image list.
https://github.com/alibaba/pouch/blob/9987860d92b48a935c3fd6caa641e96558a62d0e/test/cli_images_test.go#L114-L128

I think we should handle the table like this. It can make the test code readable and easy to understand. Just proposal.

@ZYecho
Copy link
Contributor Author

ZYecho commented Nov 22, 2018

in the test case, just use strings.Contain to skip first line of ouput, but use equal to do extractMatch, I think it's enough here.

I did one thing to parse the table into map[string][]string for image list.
pouch/test/cli_images_test.go

Lines 114 to 128 in 9987860

func imagesListToKV(list string) map[string][]string {
// skip header
lines := strings.Split(list, "\n")[1:]

res := make(map[string][]string)
for _, line := range lines {
if strings.TrimSpace(line) == "" {
continue
}

  items := strings.Fields(line) 
  res[items[1]] = items 

}
return res
}
I think we should handle the table like this. It can make the test code readable and easy to understand. Just proposal.

Yes, I come into the problem too when I impl the test case, but the other cli volume test case have the same problem, prepare to file a another pr to handle this, @fuweid WDYT?

@fuweid
Copy link
Contributor

fuweid commented Nov 22, 2018

in the test case, just use strings.Contain to skip first line of ouput, but use equal to do extractMatch, I think it's enough here.

I did one thing to parse the table into map[string][]string for image list.
pouch/test/cli_images_test.go
Lines 114 to 128 in 9987860
func imagesListToKV(list string) map[string][]string {
// skip header
lines := strings.Split(list, "\n")[1:]
res := make(map[string][]string)
for _, line := range lines {
if strings.TrimSpace(line) == "" {
continue
}

  items := strings.Fields(line) 
  res[items[1]] = items 

}
return res
}
I think we should handle the table like this. It can make the test code readable and easy to understand. Just proposal.

Yes, I come into the problem too when I impl the test case, but the other cli volume test case have the same problem, prepare to file a another pr to handle this, @fuweid WDYT?

Sure!

@fuweid fuweid merged commit c279a17 into AliyunContainerService:master Nov 22, 2018
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.

[feaure request] More flag support for pouch volume ls
7 participants