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 the format option to the docker stack ls command #31557

Merged
merged 1 commit into from Apr 27, 2017

Conversation

@boaz1337
Contributor

boaz1337 commented Mar 5, 2017

- 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

return ctx.Write(newStackContext(), render)
}
type stackContext struct {

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Mar 6, 2017

Member

MarshalJSON is needed

@AkihiroSuda

AkihiroSuda Mar 6, 2017

Member

MarshalJSON is needed

This comment has been minimized.

@boaz1337

boaz1337 Mar 6, 2017

Contributor

👍

@boaz1337

boaz1337 Mar 6, 2017

Contributor

👍

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 6, 2017

Member

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 👍

Member

vdemeester commented Mar 6, 2017

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 👍

@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 Mar 6, 2017

Contributor

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

Contributor

boaz1337 commented Mar 6, 2017

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

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 6, 2017

Member

hum, tricky, maybe ? /cc @dnephin

Member

vdemeester commented Mar 6, 2017

hum, tricky, maybe ? /cc @dnephin

@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 Mar 6, 2017

Contributor

Added documentation

Contributor

boaz1337 commented Mar 6, 2017

Added documentation

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 6, 2017

Member

Design LGTM

Member

dnephin commented Mar 6, 2017

Design LGTM

@cpuguy83

LGTM

@vdemeester

LGTM 🐮
/cc @thaJeztah for docs-review

@thaJeztah

left some minor notes, but LGTM once addressed

Show outdated Hide outdated docs/reference/commandline/stack_ls.md Outdated
Show outdated Hide outdated docs/reference/commandline/stack_ls.md Outdated
Show outdated Hide outdated docs/reference/commandline/stack_ls.md Outdated
@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 Mar 28, 2017

Contributor

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)
Contributor

boaz1337 commented Mar 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 28, 2017

Member

@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.

Member

vdemeester commented Mar 28, 2017

@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.

Add format to docker stack ls
Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>
@thaJeztah

LGTM, thanks!

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 33174 has succeeded
Details
janky Jenkins build Docker-PRs 41782 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 2024 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12959 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1848 has succeeded
Details

@thaJeztah thaJeztah added this to the 17.06 milestone Apr 27, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Apr 27, 2017

@boaz1337 boaz1337 deleted the boaz1337:add_stack_ls branch Oct 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment