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.

Sign up for GitHub

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

Jump to bottom

feature: add no-trunc flag for 'pouch ps' #909

Merged
merged 1 commit into from
Mar 16, 2018
Merged

feature: add no-trunc flag for 'pouch ps' #909

merged 1 commit into from
Mar 16, 2018

Conversation

ZouRui89
Copy link
Contributor

@ZouRui89 ZouRui89 commented Mar 16, 2018 •
edited by HusterWan
Loading

Signed-off-by: Zou Rui 21751189@zju.edu.cn

Ⅰ. Describe what this PR did

This pr adds no-trunc flag for 'pouch ps' command.

Ⅱ. Does this pull request fix one issue?

fixes #903

Ⅲ. Describe how you did it

add flag

Ⅳ. Describe how to verify it

[root@izj6c44nzox6sb97nzgmhyz ~]# pouch ps --no-trunc
Name   ID                                                                 Status        Created        Image                            Runtime
foo2   692c77587b38f60bbd91d986ec3703848d72aea5030e320d4988eb02aa3f9d48   Up 1 minute   1 minute ago   docker.io/library/redis:alpine   runc
foo    18592900006405ee64788bd108ef1de3d24dc3add73725891f4787d0f8e036f5   Up 1 minute   1 minute ago   docker.io/library/redis:alpine   runc
[root@izj6c44nzox6sb97nzgmhyz ~]# pouch ps --no-trunc -q
692c77587b38f60bbd91d986ec3703848d72aea5030e320d4988eb02aa3f9d48
18592900006405ee64788bd108ef1de3d24dc3add73725891f4787d0f8e036f5
[root@izj6c44nzox6sb97nzgmhyz ~]# pouch ps --no-trunc -a
Name   ID                                                                 Status         Created         Image                            Runtime
foo3   63fd6371f3d614bb1ecad2780972d5975ca1ab534ec280c5f7d8f4c7b2e9989d   created        2 minutes ago   docker.io/library/redis:alpine   runc
foo2   692c77587b38f60bbd91d986ec3703848d72aea5030e320d4988eb02aa3f9d48   Up 2 minutes   2 minutes ago   docker.io/library/redis:alpine   runc
foo    18592900006405ee64788bd108ef1de3d24dc3add73725891f4787d0f8e036f5   Up 2 minutes   2 minutes ago   docker.io/library/redis:alpine   runc

Ⅴ. Special notes for reviews

HusterWan
HusterWan reviewed Mar 16, 2018
View reviewed changes
cli/ps.go Outdated
@@ -65,7 +67,12 @@ func (p *PsCommand) runPs(args []string) error {

if p.flagQuiet {
for _, c := range containers {
fmt.Println(c.ID[:6])
switch p.flagNoTrunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about
id := c.ID[:6]
if p.flagNoTrunc {
id = c.ID
}

fmt.Println(id)

This will make the code more clean，WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect. Thx 👍

cli/ps.go Outdated
@@ -79,7 +86,13 @@ func (p *PsCommand) runPs(args []string) error {
return err
}

display.AddRow([]string{c.Names[0], c.ID[:6], c.Status, created + " ago", c.Image, c.HostConfig.Runtime})
switch p.flagNoTrunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@codecov-io
Copy link

codecov-io commented Mar 16, 2018 •
edited
Loading

Codecov Report

Merging #909 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #909      +/-   ##
==========================================
- Coverage    13.7%   13.66%   -0.04%     
==========================================
  Files         121      121              
  Lines        7648     7667      +19     
==========================================
  Hits         1048     1048              
- Misses       6510     6529      +19     
  Partials       90       90
Impacted Files Coverage Δ
cli/ps.go 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe6095d...f0102c4. Read the comment docs.

@HusterWan
Copy link
Contributor

@ZouRui89 Thanks for your work, we also should add some cli test case for the --no-trunc flag

@pouchrobot pouchrobot added size/M and removed size/S labels Mar 16, 2018
@ZouRui89
Copy link
Contributor Author

PTAL. @HusterWan

HusterWan
HusterWan reviewed Mar 16, 2018
View reviewed changes
test/cli_ps_test.go Outdated
res := command.PouchRun("ps", "--no-trunc").Assert(c, icmd.Success)
kv := psToKV(res.Combined())

c.Assert(kv[name].id, check.HasLen, 64)
Copy link
Contributor

Choose a reason for hiding this comment

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

The check maybe too simple:
we may execute pouch inspect CONTAINERNAME to get the ContainerID, then execute pouch ps -q --no-trunc to get the the ContainerID again, then we check whether the IDs are equaled. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Your opinion is more thoughtful. :)

@ZouRui89
feature: add no-trunc flag for 'pouch ps'
f0102c4 
Signed-off-by: Zou Rui <21751189@zju.edu.cn>
@HusterWan
Copy link
Contributor

LGTM

@HusterWan HusterWan merged commit d2c0b42 into AliyunContainerService:master Mar 16, 2018
@ZouRui89 ZouRui89 deleted the ps_add branch March 22, 2018 06:19
@pouchrobot pouchrobot mentioned this pull request Mar 23, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HusterWan HusterWan HusterWan left review comments

Assignees

@HusterWan HusterWan

Labels
kind/feature size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[feature required] pouch ps support no-trunc flag
4 participants
@ZouRui89 @codecov-io @HusterWan @pouchrobot