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

ls: format opt #1787

Merged
merged 1 commit into from
Nov 23, 2023
Merged

ls: format opt #1787

merged 1 commit into from
Nov 23, 2023

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented May 10, 2023

fixes #325
follow-up #830 with just the format opt.

$ docker buildx ls
NAME/NODE             DRIVER/ENDPOINT                                             STATUS     BUILDKIT   PLATFORMS
builder               docker-container
 \_ builder0           \_ unix:///var/run/docker.sock                             running    v0.11.6    linux/amd64, linux/amd64/v2, linux/amd64/v3, linux/arm64, linux/riscv64, linux/ppc64le
 \_ mac-mini-m1        \_ tcp://mac-mini-m1.home.foo.com:2376                     stopped               linux/arm64*
builder2*             docker-container
 \_ builder20          \_ unix:///var/run/docker.sock                             running    v0.11.6    linux/amd64, linux/amd64/v2, linux/amd64/v3, linux/arm64, linux/riscv64, linux/ppc64le
remote-builder        remote
 \_ remote-builder0    \_ docker-container://buildx_buildkit_dk-remote-builder0   inactive
 \_ aws_graviton2      \_ tcp://10.0.0.1:1234                                     running               darwin/arm64*, linux/arm64*, linux/arm/v5*, linux/arm/v6*, linux/arm/v7*, windows/arm64*
 \_ linuxone_s390x     \_ tcp://10.0.0.2:1234                                     running               linux/s390x*
default               docker
 \_ default            \_ default                                                 running    v0.8.2     linux/amd64, linux/arm64, linux/riscv64, linux/ppc64le, linux/s390x, linux/386
desktop-linux         docker
 \_ desktop-linux      \_ desktop-linux                                           error
desktop-windows       docker
 \_ desktop-windows    \_ desktop-windows                                         error
docker2010            docker
 \_ docker2010         \_ docker2010                                              running    v0.8.2     linux/amd64, linux/arm64, linux/riscv64, linux/ppc64le, linux/s390x, linux/386

Failed to get status for desktop-linux (desktop-linux): protocol not available
Failed to get status for desktop-windows (desktop-windows): protocol not available
$ docker buildx ls --format json
{"Name":"builder","Driver":"docker-container","Dynamic":false,"Nodes":[{"Name":"builder0","Endpoint":"unix:///var/run/docker.sock","Platforms":["linux/amd64","linux/amd64/v2","linux/amd64/v3","linux/arm64","linux/riscv64","linux/ppc64le","linux/s390x","linux/386","linux/mips64le","linux/mips64","linux/arm/v7","linux/arm/v6"],"Flags":["--debug","--allow-insecure-entitlement","security.insecure","--allow-insecure-entitlement","network.host"],"DriverOpts":{"env.BUILDKIT_STEP_LOG_MAX_SIZE":"10485760","env.BUILDKIT_STEP_LOG_MAX_SPEED":"10485760","env.JAEGER_TRACE":"localhost:6831","image":"moby/buildkit:master","network":"host"},"Status":"running","Version":"v0.11.6"},{"Name":"mac-mini-m1","Endpoint":"tcp://mac-mini-m1.home.foo.com:2376","DriverOpts":{"image":"moby/buildkit:master"},"Status":"stopped"}]}
{"Name":"builder2","Driver":"docker-container","Dynamic":false,"Nodes":[{"Name":"builder20","Endpoint":"unix:///var/run/docker.sock","Platforms":["linux/amd64","linux/amd64/v2","linux/amd64/v3","linux/arm64","linux/riscv64","linux/ppc64le","linux/s390x","linux/386","linux/mips64le","linux/mips64","linux/arm/v7","linux/arm/v6"],"Flags":["--debug","--allow-insecure-entitlement","security.insecure","--allow-insecure-entitlement","network.host"],"DriverOpts":{"env.BUILDKIT_STEP_LOG_MAX_SIZE":"10485760","env.BUILDKIT_STEP_LOG_MAX_SPEED":"10485760","env.JAEGER_TRACE":"localhost:6831","image":"moby/buildkit:master","network":"host"},"Status":"running","Version":"v0.11.6"}]}
{"Name":"remote-builder","Driver":"remote","Dynamic":false,"Nodes":[{"Name":"remote-builder0","Endpoint":"docker-container://buildx_buildkit_dk-remote-builder0","Status":"inactive"},{"Name":"aws_graviton2","Endpoint":"tcp://10.0.0.2:1234","Platforms":["linux/arm64","linux/arm/v7","linux/arm/v6"],"DriverOpts":{"cacert":"/home/crazy/.certs/aws_graviton2/ca.pem","cert":"/home/crazy/.certs/aws_graviton2/cert.pem","key":"/home/crazy/.certs/aws_graviton2/key.pem"},"Status":"running"},{"Name":"linuxone_s390x","Endpoint":"tcp://10.0.0.1:1234","Platforms":["linux/s390x"],"DriverOpts":{"cacert":"/home/crazy/.certs/linuxone_s390x/ca.pem","cert":"/home/crazy/.certs/linuxone_s390x/cert.pem","key":"/home/crazy/.certs/linuxone_s390x/key.pem"},"Status":"running"}]}
{"Name":"default","Driver":"docker","Dynamic":false,"Nodes":[{"Name":"default","Endpoint":"default","Platforms":["linux/amd64","linux/arm64","linux/riscv64","linux/ppc64le","linux/s390x","linux/386","linux/arm/v7","linux/arm/v6"],"Status":"running","Version":"v0.8.2"}]}
{"Name":"desktop-linux","Driver":"docker","Dynamic":false,"Nodes":[{"Name":"desktop-linux","Endpoint":"desktop-linux","Status":"error","Err":"protocol not available"}]}
{"Name":"desktop-windows","Driver":"docker","Dynamic":false,"Nodes":[{"Name":"desktop-windows","Endpoint":"desktop-windows","Status":"error","Err":"protocol not available"}]}
{"Name":"docker2010","Driver":"docker","Dynamic":false,"Nodes":[{"Name":"docker2010","Endpoint":"docker2010","Platforms":["linux/amd64","linux/arm64","linux/riscv64","linux/ppc64le","linux/s390x","linux/386","linux/arm/v7","linux/arm/v6"],"Status":"running","Version":"v0.8.2"}]}
$ docker buildx ls --format "{{.Builder.Name}}: {{range .Builder.Nodes}}{{ .Name }} {{end}}"
builder: builder0 
builder2: builder20 mac-mini-m1
remote-builder: remote-builder0 aws_graviton2 linuxone_s390x
default: default
desktop-linux: desktop-linux
desktop-windows: desktop-windows
docker2010: docker2010

@crazy-max
Copy link
Member Author

crazy-max commented May 11, 2023

I was wondering if formatting should be moved to a formatter pkg and maybe then we should have a parent cli pkg with both commands and formatter. Thoughts?

@thaJeztah
Copy link
Member

I know there's been some shuffling around in the past between "a single formatter package, containing formatting for all (sub) commands / object-types", and "per-object-type formatters"

We need to have a look what works best; the "problem" I can see with putting all formatting in a single package, is that that package would now have to depend on all object-types (packages), whereas keeping it "per-object" could (potentially) allow for more granular imports (e.g. "I only need formatting for <object-type X>).

Of course, "common" / "generic" code should be put in a place that we can share.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Also quick check (ISTR we have some commands that do "one" and some that do "the other); for the JSON output, do we want to use "jsonlines" (https://jsonlines.org), or an array?

  • I know jq is perfectly able to handle either
  • Other tools may have more difficulty with line-delimited
  • For some commands we used line-delimited, because we had to handle "streaming" responses (and didn't want to load everything into memory before being able to display (👈 but this probably doesn't apply to this command (I think we would have it all in memory, nothing streaming, right?)

commands/ls_print.go Outdated Show resolved Hide resolved
Comment on lines 57 to 58
case formatter.TableFormatKey:
format = lsDefaultTableFormat
Copy link
Member

Choose a reason for hiding this comment

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

We may have been inconsistent with this; slightly wondering if we should consider the default to be a table or not. (because of the nested presentation). We probably called it "table" for docker service ps (or which one was it?)

The default presentation COULD be considered more of a tree format (moby/moby#44582 (comment)), which could keep the road open to implement a "flat" table presentation.

(I haven't fully thought this through though!) /cc @jalonsogo

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds like major change that should be first done upstream and then tackled in cli plugins no?

commands/ls_print.go Outdated Show resolved Hide resolved
Comment on lines 55 to 56
| `.IsNode` | `true` if entry is a node |
| `.IsCurrent` | `true` if entry is the active builder |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm. I'm not sure I'm a fan of this approach for writing templates, I think we lose something from the structured data.

I think I'd rather have . be the builder, and have a .Nodes property contain a list of nodes that can be ranged or indexd to get the node details out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah me too but that's how the cli api does it today using the formatter context. I don't think there is something else than json-lines in the cli atm (cc @thaJeztah)

Copy link
Member Author

@crazy-max crazy-max May 11, 2023

Choose a reason for hiding this comment

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

Ah I see @thaJeztah commented on this #1787 (review). I would also prefer json array tbh.

Copy link
Member

Choose a reason for hiding this comment

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

@jedevc was that specific about the .IsCurrent() ? I know we added that because in the past we had * as part of the name, which was not ideal, and having a separate .IsCurrent() allow it to be "formatted how you'd like", but still keeping the raw name available to be used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah sorry my bad, comment on the wrong line 😢 I meant to comment on IsNode.

Copy link
Member Author

@crazy-max crazy-max May 11, 2023

Choose a reason for hiding this comment

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

I was looking at https://docs.docker.com/engine/reference/commandline/service_ps/ which has services and tasks (similar to us with builders and nodes) and it seems format only handles tasks and not services: https://docs.docker.com/engine/reference/commandline/service_ps/#format

@jedevc
Copy link
Collaborator

jedevc commented May 11, 2023

I was wondering if formatting should be moved to a formatter pkg and maybe then we should have a parent cli pkg with both commands and formatter. Thoughts?

IMO, we can keep it more specific for this PR, and as we grow the functionality we can pick the one that's most natural? I don't expect anyone outside of buildx to use that API (:pray: hopefully :pray:), so we should be fine to change it.

It also feels like we might want buildx inspect to support --format as well. It feels like they're quite complimentary commands, so depending on the amount of work involved, might be nice to grab that for v0.11 as well?

go.mod Outdated
@@ -120,6 +120,7 @@ require (
github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 // indirect
github.com/klauspost/compress v1.16.0 // indirect
github.com/mailru/easyjson v0.7.6 // indirect
github.com/mattn/go-runewidth v0.0.13 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

You could decide to bump this to v0.0.14 already; see docker/cli#4251 (v0.0.14 looks to have various performance improvements)

@crazy-max
Copy link
Member Author

It also feels like we might want buildx inspect to support --format as well. It feels like they're quite complimentary commands, so depending on the amount of work involved, might be nice to grab that for v0.11 as well?

Yes this is the idea: #830 (comment)

@crazy-max crazy-max added this to the v0.11.0 milestone May 11, 2023
@crazy-max crazy-max force-pushed the ls-format branch 3 times, most recently from a18a197 to 81c9973 Compare May 11, 2023 12:46
@crazy-max crazy-max removed this from the v0.11.0 milestone May 12, 2023
@crazy-max crazy-max marked this pull request as draft May 12, 2023 12:08
@crazy-max crazy-max force-pushed the ls-format branch 2 times, most recently from 223fdf2 to af757c8 Compare May 12, 2023 12:18
@crazy-max
Copy link
Member Author

Added Builder placeholder so we can access builder fields:

$ docker buildx ls --format "{{.Builder.Name}}: {{range .Builder.Nodes}}{{ .Name }} {{end}}"
builder: builder0 
builder2: builder20 mac-mini-m1
remote-builder: remote-builder0 aws_graviton2 linuxone_s390x
default: default
desktop-linux: desktop-linux
desktop-windows: desktop-windows
docker2010: docker2010

@crazy-max crazy-max force-pushed the ls-format branch 3 times, most recently from 33d7426 to a363846 Compare November 12, 2023 10:41
@crazy-max crazy-max force-pushed the ls-format branch 5 times, most recently from d8bb251 to 8cde7d1 Compare November 16, 2023 13:28
@crazy-max crazy-max marked this pull request as ready for review November 16, 2023 17:32
@crazy-max crazy-max requested review from tonistiigi and removed request for tonistiigi November 16, 2023 17:32
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

This seems to make sense to me, but I don't know if there might be compatibility issues with any other CLI commands.

berr = strings.TrimSpace(b.err.Error())
}
return json.Marshal(struct {
Name string
Copy link
Member

Choose a reason for hiding this comment

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

Just confirming, these should not be lowercase?

Copy link
Member Author

Choose a reason for hiding this comment

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

docs/reference/buildx_ls.md Outdated Show resolved Hide resolved
Signed-off-by: CrazyMax <[email protected]>
@crazy-max crazy-max merged commit cec4496 into docker:master Nov 23, 2023
63 checks passed
@crazy-max crazy-max deleted the ls-format branch November 23, 2023 14:00
@crazy-max crazy-max added this to the v0.13.0 milestone Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add --format support for command output
4 participants