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

Add -q option to `docker service ps` #27654

Merged
merged 1 commit into from Oct 28, 2016

Conversation

@gaocegege
Copy link
Contributor

commented Oct 22, 2016

- What I did

fixes #27643

- How to verify it

$ docker swarm init
$ docker service create --name redis --replicas=5 redis:3.0.6
$ docker service ps -q redis

- A picture of a cute animal (not mandatory but encouraged)

taskErr,
)
if quiet {
fmt.Fprintln(out, indentedName)

This comment has been minimized.

Copy link
@allencloud

allencloud Oct 22, 2016

Contributor

I think indentedName = fmt.Sprintf(" \\_ %s", indentedName) above may be displayed via this line of code. If that, I have no idea if it is reasonable. WDYT @gaocegege

In addition, if quiet, maybe we do not have to process the err things. Just print when constructing of indentedName finishes. 😄

This comment has been minimized.

Copy link
@gaocegege

gaocegege Oct 22, 2016

Author Contributor

In addition, if quiet, maybe we do not have to process the err things. Just print when constructing of indentedName finishes. 😄

🤔 Yeah, there is no need to process errs.

I think indentedName = fmt.Sprintf(" \\_ %s", indentedName) above may be displayed via this line of code. If that, I have no idea if it is reasonable. WDYT @gaocegege

Ah, okay I think we should pass the indent logic when ps -q

        if quiet {
            fmt.Fprintln(out, name)
            continue
        }
        // indented name logic

        // err processing logic

Then the result would be

redis.1.mwvj0azq00lcydntsv79uj2k6
-  \_ redis.1.zk9fhepa7cgnoex3pgn0sngir
+ redis.1.zk9fhepa7cgnoex3pgn0sngir

Or do you have a better idea about indented name? 😃

@allencloud

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2016

We need a -q option that removes headers and only does ID.

I am afraid redis.1. in redis.1.6u87kp12lrw1ikrejqti51g8y is not included in task id. @gaocegege

@gaocegege

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2016

PTAL

$ docker service ps redis
NAME                               IMAGE        NODE          DESIRED STATE  CURRENT STATE            ERROR
redis.1.xrlbr5skxm5mfquugv7igtw21  redis:3.0.6  56a8544282d8  Running        Preparing 9 seconds ago
redis.2.69bi767slr5fmpb37n78zop4m  redis:3.0.6  56a8544282d8  Running        Preparing 9 seconds ago
$ docker service ps -q redis
xrlbr5skxm5mfquugv7igtw21
69bi767slr5fmpb37n78zop4m

@cpuguy83 cpuguy83 changed the title fixes #27643 Add -q option to `docker service ps` Oct 24, 2016

@LK4D4

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2016

LGTM
I'm not sure what to do with docs, though. ping @thaJeztah

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

I'm not sure what to do with docs, though.

This would only need changes to the reference docs (man-page for this is generated); @gaocegege can you update https://github.com/docker/docker/blob/master/docs/reference/commandline/service_ps.md ?

It also needs a change to the bash and zsh completion scripts, so /cc @albers @sdurrheimer

@gaocegege

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2016

re @thaJeztah

I have updated the docs. And I found there is no -a option now, so I removed it and add -q.

preview

I would fix the bash/zsh/fish completion now.

@gaocegege gaocegege changed the title Add -q option to `docker service ps` [WIP] Add -q option to `docker service ps` Oct 25, 2016

@gaocegege gaocegege changed the title [WIP] Add -q option to `docker service ps` Add -q option to `docker service ps` Oct 26, 2016

@sdurrheimer

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2016

zsh completion LGTM

@@ -2462,7 +2462,7 @@ _docker_service_ps() {

case "$cur" in
-*)
COMPREPLY=( $( compgen -W "--all -a --filter -f --help --no-resolve --no-trunc" -- "$cur" ) )
COMPREPLY=( $( compgen -W "--quiet -q --filter -f --help --no-resolve --no-trunc" -- "$cur" ) )

This comment has been minimized.

Copy link
@albers

albers Oct 26, 2016

Member

Please preserve alphabetical order here.

This comment has been minimized.

Copy link
@gaocegege

gaocegege Oct 26, 2016

Author Contributor

OK

@@ -1167,7 +1167,7 @@ __docker_service_subcommand() {
(ps)
_arguments $(__docker_arguments) \
$opts_help \
"($help -a --all)"{-a,--all}"[Display all tasks]" \
"($help -q --quiet)"{-q,--quiet}"[Only display IDs]" \

This comment has been minimized.

Copy link
@sdurrheimer

sdurrheimer Oct 26, 2016

Contributor

I didn't notice it the first time, but same as @albers comment, try to keep alphabetical order.

This comment has been minimized.

Copy link
@gaocegege

gaocegege Oct 26, 2016

Author Contributor

OK

@albers
albers approved these changes Oct 26, 2016
Copy link
Member

left a comment

bash completion LGTM, Thanks!

@sdurrheimer
Copy link
Contributor

left a comment

re-LGTM

@vdemeester
Copy link
Member

left a comment

LGTM 👼
/cc @aaronlehmann @stevvooe

}

// PrintQuiet shows task list in a quiet way.
func PrintQuiet(dockerCli *command.DockerCli, tasks []swarm.Task) error {

This comment has been minimized.

Copy link
@stevvooe

stevvooe Oct 27, 2016

Contributor

No need to export this.

This comment has been minimized.

Copy link
@aaronlehmann

This comment has been minimized.

Copy link
@stevvooe

stevvooe Oct 27, 2016

Contributor

I see: this package is useless.

Just move print.go to the service package. This isn't used elsewhere.

This comment has been minimized.

Copy link
@gaocegege

gaocegege Oct 27, 2016

Author Contributor

ah, the function task.Print is called from https://github.com/docker/docker/blob/master/cli/command/node/ps.go#L84 and https://github.com/docker/docker/blob/master/cli/command/stack/ps.go#L69. If we move task/print.go, these would import the service package instead. 🤔

This comment has been minimized.

Copy link
@stevvooe

stevvooe Oct 27, 2016

Contributor

No reason to preserve the package import. These functions are a for loop and println.

Perhaps, this is outside the scope of this pr. Let's leave as is and fix in another pr.

@stevvooe
Copy link
Contributor

left a comment

Please don't export functions that don't need to be exported.

@stevvooe

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2016

LGTM

If someone wants de-leftpad the task package, that would be awesome!

@vdemeester

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

Moving this to docs-review then 👼
/cc @thaJeztah

@@ -32,6 +33,7 @@ func newPsCommand(dockerCli *command.DockerCli) *cobra.Command {
},
}
flags := cmd.Flags()
flags.BoolVarP(&opts.quiet, "quiet", "q", false, "Only display IDs")

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 27, 2016

Member

Looks good, wondering if we should use Only display task IDs (there's so many ID's related to this overview, it became a bit confusing to me)

@vdemeester wdyt?

This comment has been minimized.

Copy link
@vdemeester

vdemeester Oct 27, 2016

Member

ah right, I do think Only display task IDs would make it a little bit clearer indeed 👼

This comment has been minimized.

Copy link
@gaocegege

gaocegege Oct 27, 2016

Author Contributor

Ah, OK

I would add it.


List the tasks of a service

Options:
-a, --all Display all tasks

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 27, 2016

Member

heh, looks like I copy-pasted that, thanks for catching!

@@ -40,7 +41,8 @@ func (t tasksBySlot) Less(i, j int) bool {
return t[j].Meta.CreatedAt.Before(t[i].CreatedAt)
}

// Print task information in a table format
// Print task information in a table forgmat.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 27, 2016

Member

typo; forgmat

@cpuguy83

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2016

Can you squash commits, please?

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Oct 28, 2016

LGTM after commits are squashed

fixes #27643
Signed-off-by: Ce Gao <ce.gao@outlook.com>
@vdemeester
Copy link
Member

left a comment

LGTM 🐸

vdemeester added a commit that referenced this pull request Oct 28, 2016
Merge pull request #27654 from gaocegege/add-quiet-mode-to-service-ps
Add -q option to `docker service ps`

@vdemeester vdemeester merged commit 8901197 into moby:master Oct 28, 2016

4 checks passed

docker/dco-signed All commits signed
Details
experimental Jenkins build Docker-PRs-experimental 25597 has succeeded
Details
janky Jenkins build Docker-PRs 34195 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 5084 has succeeded
Details

@vdemeester vdemeester merged commit 9efa472 into moby:master Oct 28, 2016

4 checks passed

docker/dco-signed All commits signed
Details
experimental Jenkins build Docker-PRs-experimental 25597 has succeeded
Details
janky Jenkins build Docker-PRs 34195 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 5084 has succeeded
Details
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
Merge pull request moby#27654 from gaocegege/add-quiet-mode-to-servic…
…e-ps

Add -q option to `docker service ps`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.