Add the format option to the docker stack ls command by boaz0 · Pull Request #31557 · moby/moby

boaz0 · 2017-03-05T18:05:29Z

- What I did
Add the format option to the docker-stack-ls command
related to #30431

- How I did it

Add the format option to the cli/command/stack/ls.go
Create cli/command/formatter/stack.go
Move stack.stack to api.types.Stack
Add unit tests

- How to verify it
TESTDIRS='cli/command/formatter/' TESTFLAGS='-test.run ^TestStack*' hack/make.sh test-unit

AkihiroSuda · 2017-03-06T03:49:33Z

cli/command/formatter/stack.go

MarshalJSON is needed

vdemeester · 2017-03-06T08:44:01Z

Move stack.stack to api.types.Stack

This ain't right, at that point stack are only a client thing (a cli thing even) and thus, shouldn't be in the api package as it is not (yet) part of the API.

Otherwise, design LGTM for me 👍

boaz0 · 2017-03-06T09:09:14Z

@vdemeester thanks for the feedback.
I guess I will move it from api/types to cli/command/formatter.
Does that make sense?

vdemeester · 2017-03-06T09:28:36Z

hum, tricky, maybe ? /cc @dnephin

boaz0 · 2017-03-06T15:00:44Z

Added documentation

dnephin · 2017-03-06T16:28:28Z

Design LGTM

allencloud · 2017-03-07T02:15:55Z

docs/reference/commandline/stack_ls.md

Are both the additional lines intentionally added?

No. I will remove those two. Thanks for noticing!
👍

AkihiroSuda · 2017-03-13T09:17:32Z

docs/reference/commandline/stack_ls.md

stacks output -> stacks

AkihiroSuda · 2017-03-13T09:17:56Z

docs/reference/commandline/stack_ls.md

not docker network 😄

😱 what the hell is wrong with me.
Thank you for noticing.

cpuguy83

LGTM

vdemeester

LGTM 🐮
/cc @thaJeztah for docs-review

thaJeztah

left some minor notes, but LGTM once addressed

thaJeztah · 2017-03-28T11:11:51Z

docs/reference/commandline/stack_ls.md

s/networks/stacks/

thaJeztah · 2017-03-28T11:12:02Z

docs/reference/commandline/stack_ls.md

s/options/option/

thaJeztah · 2017-03-28T11:13:01Z

docs/reference/commandline/stack_ls.md

Can you change "will either output" to "either outputs" ?

boaz0 · 2017-03-28T11:52:38Z

That's odd. There is a compilation error:

11:47:00 ---> Making bundle: dynbinary (in bundles/17.05.0-dev/dynbinary)
11:47:00 Building: bundles/17.05.0-dev/dynbinary-client/docker-17.05.0-dev
11:47:11 # github.com/docker/docker/cli/command/stack
11:47:11 cli/command/stack/list.go:4: imported and not used: "fmt"

It says that in cli/command/stack/list.go the fmt package is imported but isn't used but that's not true because on line 77 fmt.Errorf is called.

return nil, fmt.Errorf("cannot get label %s for service %s", convert.LabelNamespace, service.ID)

vdemeester · 2017-03-28T12:03:50Z

@ripcurld0 I think you should rebase your code against master 👼 Guess something has been merged that uses pkg/errors instead of fmt for errors, I guess this is what make the build fail.

AkihiroSuda · 2017-04-14T03:07:27Z

integration-cli/docker_cli_stack_test.go

Why do we need this change?
Also, it would be better to replace whitespace sequence to a single whitespace before comparison

I had to change it because the tabwriter in formatter/formatter.go is different from the one that was used in stack/list.go.

Yep, that sounds like a good idea! I will do that 👍

Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>

thaJeztah

LGTM, thanks!

thaJeztah · 2017-04-27T02:14:09Z

/cc @albers @sdurrheimer

GordonTheTurtle added the status/0-triage label Mar 5, 2017

boaz0 mentioned this pull request Mar 5, 2017

[UX] implementation status of the --format #30431

Closed

24 tasks

AkihiroSuda added status/1-design-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/0-triage labels Mar 6, 2017

AkihiroSuda reviewed Mar 6, 2017

View reviewed changes

dnephin added status/2-code-review and removed status/1-design-review labels Mar 6, 2017

yongtang removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Mar 6, 2017

allencloud reviewed Mar 7, 2017

View reviewed changes

AkihiroSuda reviewed Mar 13, 2017

View reviewed changes

docs/reference/commandline/stack_ls.md Outdated

Copy link

Member

AkihiroSuda Mar 13, 2017

Choose a reason for hiding this comment

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

stacks output -> stacks

boaz0 reacted with thumbs up emoji

AkihiroSuda reviewed Mar 13, 2017

View reviewed changes

AkihiroSuda added the rebuild/powerpc label Mar 14, 2017

GordonTheTurtle removed the rebuild/powerpc label Mar 14, 2017

GordonTheTurtle assigned aluzzardi Mar 20, 2017

cpuguy83 approved these changes Mar 23, 2017

View reviewed changes

vdemeester approved these changes Mar 24, 2017

View reviewed changes

vdemeester added status/3-docs-review and removed status/2-code-review labels Mar 24, 2017

thaJeztah requested changes Mar 28, 2017

View reviewed changes

AkihiroSuda reviewed Apr 14, 2017

View reviewed changes

Add format to docker stack ls

205ec49

Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>

thaJeztah added the impact/changelog label Apr 27, 2017

thaJeztah added the impact/cli label Apr 27, 2017

thaJeztah approved these changes Apr 27, 2017

View reviewed changes

thaJeztah merged commit 6559aba into moby:master Apr 27, 2017

thaJeztah added this to the 17.06 milestone Apr 27, 2017

albers mentioned this pull request Apr 27, 2017

Add bash completion for stack ls --format #32882

Merged

boaz0 deleted the add_stack_ls branch October 6, 2017 14:24

Conversation

boaz0 commented Mar 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Mar 6, 2017

Uh oh!

boaz0 commented Mar 6, 2017

Uh oh!

vdemeester commented Mar 6, 2017

Uh oh!

boaz0 commented Mar 6, 2017

Uh oh!

dnephin commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boaz0 commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

boaz0 commented Mar 5, 2017 •

edited

Loading

boaz0 commented Mar 28, 2017 •

edited

Loading