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 formatter for service inspect #25025

Merged
merged 1 commit into from Sep 20, 2016

Conversation

Projects
None yet
7 participants
@cpuguy83
Contributor

cpuguy83 commented Jul 25, 2016

Allows the user to use pretty as the format string.
This enables users to put custom format options into their CLI config
just like is supported for docker ps and docker images

Example config:

{
    "psFormat": "table {{.Names}}\\t{{.Image}}\\t{{.RunningFor}} ago\\t{{.Status}}\\t{{.Command}}",
    "imagesFormat": "table {{.Repository}}\\t{{.Tag}}\\t{{.ID}}\\t{{.Size}}",
    "serviceInspectFormat": "pretty"
}
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
Contributor

cpuguy83 commented Jul 25, 2016

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jul 25, 2016

Contributor

I'm still kind of in shock that we're having service inspect print out JSON without a special option. I understand we're doing it to be consistent with docker inspect. But it sounds like docker inspect was originally a debugging tool, and now we're stuck with its output format. Since docker service inspect is new, it seems highly unfortunate to propagate this. It's particularly problematic since docker inspect is not commonly used in everyday use cases, but becuase of the declarative model, docker service inspect is important for checking on the service's status; for example, whether an update is running or paused.

I appreciate the change here to give people a workaround, but I think it doesn't fix the fundamental usability issue. I would prefer docker service inspect to not print JSON by default, and if the deviation from docker inspect isn't acceptable, we should rename it to something other than inspect so we aren't locked into unreadable JSON output.

Contributor

aaronlehmann commented Jul 25, 2016

I'm still kind of in shock that we're having service inspect print out JSON without a special option. I understand we're doing it to be consistent with docker inspect. But it sounds like docker inspect was originally a debugging tool, and now we're stuck with its output format. Since docker service inspect is new, it seems highly unfortunate to propagate this. It's particularly problematic since docker inspect is not commonly used in everyday use cases, but becuase of the declarative model, docker service inspect is important for checking on the service's status; for example, whether an update is running or paused.

I appreciate the change here to give people a workaround, but I think it doesn't fix the fundamental usability issue. I would prefer docker service inspect to not print JSON by default, and if the deviation from docker inspect isn't acceptable, we should rename it to something other than inspect so we aren't locked into unreadable JSON output.

Show outdated Hide outdated api/client/cli.go
// String contains columns and format specification, for example {{ID}}\t{{Name}}.
func (cli *DockerCli) ServiceInspectFormat() string {
return cli.configFile.ServiceInspectFormat
}

This comment has been minimized.

@dnephin

dnephin Jul 25, 2016

Member

I made a similar comment on another PR but I guess we haven't cleaned this up yet. I don't think we should be exposing the formatter directly from the DockerCli object.

We already have cli.ConfigFile() we should just use that and access the ServiceInspectFormat directly from the configFile object.

@dnephin

dnephin Jul 25, 2016

Member

I made a similar comment on another PR but I guess we haven't cleaned this up yet. I don't think we should be exposing the formatter directly from the DockerCli object.

We already have cli.ConfigFile() we should just use that and access the ServiceInspectFormat directly from the configFile object.

This comment has been minimized.

@vdemeester

vdemeester Jul 26, 2016

Member

@dnephin the PR is not merged either 👼 (waiting for the release to update and merge)

@vdemeester

vdemeester Jul 26, 2016

Member

@dnephin the PR is not merged either 👼 (waiting for the release to update and merge)

Show outdated Hide outdated api/client/formatter/service.go
units "github.com/docker/go-units"
)
const serviceInspectPrettyTemplate = `

This comment has been minimized.

@dnephin

dnephin Jul 25, 2016

Member

+1 thanks for making this change.

Now that I know about {{- }} I can clean up the cobra usage template.

@dnephin

dnephin Jul 25, 2016

Member

+1 thanks for making this change.

Now that I know about {{- }} I can clean up the cobra usage template.

This comment has been minimized.

@thaJeztah

thaJeztah Jul 26, 2016

Member

we should add some examples to https://docs.docker.com/engine/admin/formatting/

(I also didn't know of {{- }})

@thaJeztah

thaJeztah Jul 26, 2016

Member

we should add some examples to https://docs.docker.com/engine/admin/formatting/

(I also didn't know of {{- }})

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jul 25, 2016

Member

By making it a format, and putting the format into the config, users can enable the pretty format by default if they want to. We should make sure this gets documented in the cli reference. I think this needs to be consistent with docker inspect. It's possible in 1.13 we could hide inspect, and introduce a new command (maybe show ?) that defaults to pretty and provides the json as a flag. That way we'd only have one command for printing, and we can make the default pretty without breaking any scripts. But we should do that for all the inspect commands at once, and I don't think we have time to make that change for 1.12.

docker service inspect -f=pretty isn't that bad. People are used to getting JSON and passing -f to format it.

Design LGTM

Member

dnephin commented Jul 25, 2016

By making it a format, and putting the format into the config, users can enable the pretty format by default if they want to. We should make sure this gets documented in the cli reference. I think this needs to be consistent with docker inspect. It's possible in 1.13 we could hide inspect, and introduce a new command (maybe show ?) that defaults to pretty and provides the json as a flag. That way we'd only have one command for printing, and we can make the default pretty without breaking any scripts. But we should do that for all the inspect commands at once, and I don't think we have time to make that change for 1.12.

docker service inspect -f=pretty isn't that bad. People are used to getting JSON and passing -f to format it.

Design LGTM

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Jul 26, 2016

Contributor

If we make pretty the default in the future, how would you get the json instead? -f json?

Contributor

aluzzardi commented Jul 26, 2016

If we make pretty the default in the future, how would you get the json instead? -f json?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jul 26, 2016

Contributor

@aluzzardi yes.

Contributor

cpuguy83 commented Jul 26, 2016

@aluzzardi yes.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Aug 18, 2016

Member

I think we need a format flag here, so I'm moving this to code review

Member

thaJeztah commented Aug 18, 2016

I think we need a format flag here, so I'm moving this to code review

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Aug 25, 2016

Member

LGTM 🐯

Member

vdemeester commented Aug 25, 2016

LGTM 🐯

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 9, 2016

Contributor

Rebased

Contributor

cpuguy83 commented Sep 9, 2016

Rebased

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 13, 2016

Contributor

Ping. This should be a fairly easy review, but tricky to update when new fields are added in other PRs.

Contributor

cpuguy83 commented Sep 13, 2016

Ping. This should be a fairly easy review, but tricky to update when new fields are added in other PRs.

Show outdated Hide outdated cli/command/formatter/service.go
serviceI, _, err := ctx.GetRef(ref)
if err != nil {
return err
}

This comment has been minimized.

@dnephin

dnephin Sep 13, 2016

Member

I don't know about this. It seems very strange to me that a formatting Context would fetch the objects.

All the other formatters take a slice of objects and operate on them, why does this need to call the GetRef function?

@dnephin

dnephin Sep 13, 2016

Member

I don't know about this. It seems very strange to me that a formatting Context would fetch the objects.

All the other formatters take a slice of objects and operate on them, why does this need to call the GetRef function?

This comment has been minimized.

@cpuguy83

cpuguy83 Sep 13, 2016

Contributor

I took this exactly from container inspect, or maybe it was container ps.

@cpuguy83

cpuguy83 Sep 13, 2016

Contributor

I took this exactly from container inspect, or maybe it was container ps.

This comment has been minimized.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 13, 2016

Member

I guess this works. I think the Context should not be responsible for fetching the objects, but I understand this is the first time we're mixing the inspector with the formatter. They are both very similar, so I think it makes sense, and should eventually allow us to delete some duplicate code (once all objects can use the formatter for inspect).

I'm working on a big refactor to the formatter package, so I can address it as part of that.

LGTM

I noticed something else, taking it back

Member

dnephin commented Sep 13, 2016

I guess this works. I think the Context should not be responsible for fetching the objects, but I understand this is the first time we're mixing the inspector with the formatter. They are both very similar, so I think it makes sense, and should eventually allow us to delete some duplicate code (once all objects can use the formatter for inspect).

I'm working on a big refactor to the formatter package, so I can address it as part of that.

LGTM

I noticed something else, taking it back

Show outdated Hide outdated cli/command/formatter/service.go
}
func (ctx *serviceInspectContext) ContainerUser() string {
return ctx.Service.Spec.TaskTemplate.ContainerSpec.Dir

This comment has been minimized.

@dnephin

dnephin Sep 13, 2016

Member

This looks wrong.

@dnephin

dnephin Sep 13, 2016

Member

This looks wrong.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 13, 2016

Contributor

Fixed and green

Contributor

cpuguy83 commented Sep 13, 2016

Fixed and green

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 13, 2016

Member

LGTM

Member

dnephin commented Sep 13, 2016

LGTM

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 13, 2016

Contributor

@vdemeester Still good for you?

Contributor

cpuguy83 commented Sep 13, 2016

@vdemeester Still good for you?

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Sep 13, 2016

Member

@cpuguy83 still LGTM 👼

Member

vdemeester commented Sep 13, 2016

@cpuguy83 still LGTM 👼

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Sep 13, 2016

Member

Moving to docs-review 👼

Member

vdemeester commented Sep 13, 2016

Moving to docs-review 👼

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 14, 2016

Contributor

Added doc change to mention the serviceInspectFormat key in the CLI config file.

Contributor

cpuguy83 commented Sep 14, 2016

Added doc change to mention the serviceInspectFormat key in the CLI config file.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 18, 2016

Contributor

Rebased, had to change code for the new formatter changes.

Contributor

cpuguy83 commented Sep 18, 2016

Rebased, had to change code for the new formatter changes.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 19, 2016

Member

@cpuguy83 tests are failing;

19:05:37 # github.com/docker/docker/cli/command/service
19:05:37 cli/command/service/inspect_test.go:81: undefined: formatter.ServiceInspectContext
Member

thaJeztah commented Sep 19, 2016

@cpuguy83 tests are failing;

19:05:37 # github.com/docker/docker/cli/command/service
19:05:37 cli/command/service/inspect_test.go:81: undefined: formatter.ServiceInspectContext
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 19, 2016

Member

We should probably mention the pretty option for --format as well in the CLI reference; although I'm not really sure how this interacts with / when it should be used instead of --pretty; https://github.com/docker/docker/blob/master/docs/reference/commandline/service_inspect.md#inspect-a-service-using-pretty-print

Member

thaJeztah commented Sep 19, 2016

We should probably mention the pretty option for --format as well in the CLI reference; although I'm not really sure how this interacts with / when it should be used instead of --pretty; https://github.com/docker/docker/blob/master/docs/reference/commandline/service_inspect.md#inspect-a-service-using-pretty-print

property is not set, the client falls back to the default json format. For a
list of supported formatting directives, see the
[**Formatting** section in the `docker service inspect` documentation](service_inspect.md)
Following is a sample `config.json` file:
{

This comment has been minimized.

@thaJeztah

thaJeztah Sep 19, 2016

Member

Can you add an example here?

@thaJeztah

thaJeztah Sep 19, 2016

Member

Can you add an example here?

This comment has been minimized.

@cpuguy83

cpuguy83 Sep 19, 2016

Contributor

done

@cpuguy83

cpuguy83 Sep 19, 2016

Contributor

done

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 19, 2016

Contributor

@thaJeztah It should never really be used instead of --pretty. It's only added so the default output can be set to pretty in the json config.

Contributor

cpuguy83 commented Sep 19, 2016

@thaJeztah It should never really be used instead of --pretty. It's only added so the default output can be set to pretty in the json config.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 19, 2016

Member

Thanks! docs LGTM

Member

thaJeztah commented Sep 19, 2016

Thanks! docs LGTM

@thaJeztah thaJeztah added this to the 1.13.0 milestone Sep 19, 2016

Add formatter for service inspect
Allows the user to use `pretty` as the format string.
This enables users to put custom format options into their CLI config
just like is supported for `docker ps` and `docker images`

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 20, 2016

Member

ping @vdemeester PTAL for docs

Member

thaJeztah commented Sep 20, 2016

ping @vdemeester PTAL for docs

@vdemeester

docs LGTM 🐮

@vdemeester vdemeester merged commit 00615ef into moby:master Sep 20, 2016

7 checks passed

docker/dco-signed All commits signed
Details
documentation success
Details
experimental Jenkins build Docker-PRs-experimental 23937 has succeeded
Details
janky Jenkins build Docker-PRs 32527 has succeeded
Details
userns Jenkins build Docker-PRs-userns 14541 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 31276 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 3327 has succeeded
Details

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

@cpuguy83 cpuguy83 deleted the cpuguy83:service_inspect_formatter branch Sep 20, 2017

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