ls: add format, quiet and no-trunc opts #830
Conversation
|
I'll try to have a closer look, but some quick thoughts
Hm.. yes, so I suggested that indeed. I think my initial thought there was that the problem in the current output was that the error message took the place of the Looking at your example output, I'm now wondering if (instead) it would make sense to add an Similar to docker service create --name foobar nginx:alpine
docker service ps foobar
ID NAME IMAGE NODE DESIRED STATE CURRENT STATE ERROR PORTS
a9b0sgzs04ej foobar.1 nginx:alpine docker-desktop Running Running 11 seconds ago
docker service create --name failing --detach busybox sh -c 'sleep 2; exit 1'
docker service ps failing
ID NAME IMAGE NODE DESIRED STATE CURRENT STATE ERROR PORTS
jskpciju2hpw failing.1 busybox:latest docker-desktop Ready Ready 1 second ago
mfl1c0dr88wf \_ failing.1 busybox:latest docker-desktop Shutdown Failed 1 second ago "task: non-zero exit (1)"
wmg9tn7w6btk \_ failing.1 busybox:latest docker-desktop Shutdown Failed 9 seconds ago "task: non-zero exit (1)"I think
Yes, I'm a bit on the fence on that one. That said, "it's tricky" Something "in between" would be to have a way to stay "attached" (like Overall, I'm wondering if we could find some middle-ground where I'm also looking if we can format the output more so that the "essential" information can be presented as "one row per builder", instead of the staggered (multiple lines per builder) output, which makes it hard to see "at a glance" what nodes I have, and to find the information. Currently we have;
Of those;
|
crazy-max
changed the title
|
@thaJeztah Switched to cli formatter. See updated PR description. |
| return json.Marshal(struct { | ||
| Name string | ||
| Endpoint string | ||
| Platforms []string `json:",omitempty"` |
There was a problem hiding this comment.
I know we haven't been very consistent in these, but wondering if we should include an explicit name to use in the json output; wdyt? i.e., if we explicitly want the output to be starting with an uppercase;
| Platforms []string `json:",omitempty"` | |
| Platforms []string `json:"Platforms,omitempty"` |
| var pp []string | ||
| for _, p := range n.Platforms { | ||
| pp = append(pp, platforms.Format(p)) | ||
| } | ||
| return json.Marshal(struct { |
There was a problem hiding this comment.
I was initially wondering if we should have an actual type for this; possibly if we need specific formatting functions for printing, we could define those on the type.
Then I wondered what we needed the ad-hoc type for (was it because of recursion happening during MarshalJSON?); but then noticed the platforms handling above.
Do you know what specifically was needed to use the platforms.Format()? Out of curiousity, I tried what happened if we use []ocispecs.Platform as-is, and (at a glance
This is still using the ad-hoc type, but updating it to use OCI platform;
diff --git a/builder/node.go b/builder/node.go
index 665413f7..2954b153 100644
--- a/builder/node.go
+++ b/builder/node.go
@@ -5,7 +5,6 @@ import (
"encoding/json"
"strings"
- "github.com/containerd/containerd/platforms"
"github.com/docker/buildx/driver"
ctxkube "github.com/docker/buildx/driver/kubernetes/context"
"github.com/docker/buildx/store"
@@ -176,25 +175,21 @@ func (n *Node) MarshalJSON() ([]byte, error) {
status = "error"
err = strings.TrimSpace(n.Err.Error())
}
- var pp []string
- for _, p := range n.Platforms {
- pp = append(pp, platforms.Format(p))
- }
return json.Marshal(struct {
Name string
Endpoint string
- Platforms []string `json:",omitempty"`
- Flags []string `json:",omitempty"`
- DriverOpts map[string]string `json:",omitempty"`
- Files map[string][]byte `json:",omitempty"`
- Status string `json:",omitempty"`
- ProxyConfig map[string]string `json:",omitempty"`
- Version string `json:",omitempty"`
- Err string `json:",omitempty"`
+ Platforms []ocispecs.Platform `json:",omitempty"`
+ Flags []string `json:",omitempty"`
+ DriverOpts map[string]string `json:",omitempty"`
+ Files map[string][]byte `json:",omitempty"`
+ Status string `json:",omitempty"`
+ ProxyConfig map[string]string `json:",omitempty"`
+ Version string `json:",omitempty"`
+ Err string `json:",omitempty"`
}{
Name: n.Name,
Endpoint: n.Endpoint,
- Platforms: pp,
+ Platforms: n.Platforms,
Flags: n.Flags,
DriverOpts: n.DriverOpts,
Files: n.Files,There was a problem hiding this comment.
With the above, the output seems to be fine (but perhaps there's specific corner-cases we're not accounting for?)
./bin/build/docker-buildx ls
NAME/NODE DRIVER/ENDPOINT STATUS BUILDKIT PLATFORMS
default * docker
\_ default \_ default running 22.06.0-beta.0-902-g2708be0db4.m linux/arm64, linux/amd64, linux/amd64/v2, linux/riscv64, linux/ppc64le, linux/s390x,…
desktop-linux docker
\_ desktop-linux \_ desktop-linux running 22.06.0-beta.0-902-g2708be0db4.m linux/arm64, linux/amd64, linux/amd64/v2, linux/riscv64, linux/ppc64le, linux/s390x,…
foo docker
\_ foo \_ foo error
Failed to get status for foo (foo): Unix socket path "/var/folders/6f/tz5jf4nn1_n5jb0ctrrw5p2w0000gn/T/TestBuildWithBuildercustom_context67217891/001/docker.sock" is too long👆 😂 Ah, fun; looks like I ran some integration test on my machine, and macOS "temp" directories are symlinks, and the actual location looks to be too long.
I also tried outputing only the .Platforms, which also seems to work, although (not sure where those come from), it looks like there's empty lines in between each row 🤔
./bin/build/docker-buildx ls --format '{{.Platforms}}'
linux/arm64, linux/amd64, linux/amd64/v2, linux/riscv64, linux/ppc64le, linux/s390x, linux/mips64le, linux/mips64
linux/arm64, linux/amd64, linux/amd64/v2, linux/riscv64, linux/ppc64le, linux/s390x, linux/mips64le, linux/mips64
There was a problem hiding this comment.
but perhaps there's specific corner-cases we're not accounting for?
Oh! Perhaps this was for the JSON format? As without it, it would render the platform as a struct? (Wondering if perhaps we should actually keep that 🤔)
./bin/build/docker-buildx ls --format json
{"Name":"default","Driver":"docker","Dynamic":false,"Nodes":[{"Name":"default","Endpoint":"default","Platforms":[{"architecture":"arm64","os":"linux"},{"architecture":"amd64","os":"linux"},{"architecture":"amd64","os":"linux","variant":"v2"},{"architecture":"riscv64","os":"linux"},{"architecture":"ppc64le","os":"linux"},{"architecture":"s390x","os":"linux"},{"architecture":"mips64le","os":"linux"},{"architecture":"mips64","os":"linux"}],"Status":"running","Version":"22.06.0-beta.0-902-g2708be0db4.m"}]}
{"Name":"desktop-linux","Driver":"docker","Dynamic":false,"Nodes":[{"Name":"desktop-linux","Endpoint":"desktop-linux","Platforms":[{"architecture":"arm64","os":"linux"},{"architecture":"amd64","os":"linux"},{"architecture":"amd64","os":"linux","variant":"v2"},{"architecture":"riscv64","os":"linux"},{"architecture":"ppc64le","os":"linux"},{"architecture":"s390x","os":"linux"},{"architecture":"mips64le","os":"linux"},{"architecture":"mips64","os":"linux"}],"Status":"running","Version":"22.06.0-beta.0-902-g2708be0db4.m"}]}
{"Name":"foo","Driver":"docker","Dynamic":false,"Nodes":[{"Name":"foo","Endpoint":"foo","Status":"error","Err":"Unix socket path \"/var/folders/6f/tz5jf4nn1_n5jb0ctrrw5p2w0000gn/T/TestBuildWithBuildercustom_context67217891/001/docker.sock\" is too long"}]}There was a problem hiding this comment.
Oh, and I guess it doesn't show the default / native platform; (linux/arm64* vs linux/arm64);
edit:
If we really want a string presentation in all cases, we could have a local type that wraps oci.Platform and implements MarshalText;
diff --git a/builder/node.go b/builder/node.go
index 665413f7..5ee00381 100644
--- a/builder/node.go
+++ b/builder/node.go
@@ -166,6 +166,12 @@ func (b *Builder) LoadNodes(ctx context.Context, withData bool) (_ []Node, err e
return b.nodes, nil
}
+type Platform ocispecs.Platform
+
+func (p *Platform) MarshalText() ([]byte, error) {
+ return []byte(platforms.Format(ocispecs.Platform(*p))), nil
+}
+
func (n *Node) MarshalJSON() ([]byte, error) {
var status string
if n.DriverInfo != nil {
@@ -176,14 +182,14 @@ func (n *Node) MarshalJSON() ([]byte, error) {
status = "error"
err = strings.TrimSpace(n.Err.Error())
}
- var pp []string
+ var pp []Platform
for _, p := range n.Platforms {
- pp = append(pp, platforms.Format(p))
+ pp = append(pp, Platform(p))
}
return json.Marshal(struct {
Name string
Endpoint string
- Platforms []string `json:",omitempty"`
+ Platforms []Platform `json:",omitempty"`
Flags []string `json:",omitempty"`
DriverOpts map[string]string `json:",omitempty"`
Files map[string][]byte `json:",omitempty"`There was a problem hiding this comment.
Do you know what specifically was needed to use the
platforms.Format()?
This is for the --format json so we are consistent with table / raw result.
I also tried outputing only the
.Platforms, which also seems to work, although (not sure where those come from), it looks like there's empty lines in between each row 🤔
Blank lines are the builders that don't carry platforms (only nodes). I'm not sure how we can handle this with custom go template as builders and nodes are really different.
Oh, and I guess it doesn't show the
default/nativeplatform; (linux/arm64*vslinux/arm64);
* on platform means user enforces this platform when creating the builder or appending a node (create --platform linux/amd64) so it will build on this node if it matches the target platform. Atm table, raw format shows supported and preferred platforms but json format doesn't (maybe it should?).
|
For the ./bin/build/docker-buildx ls --format raw
name: default *
driver: docker
nodes:
name: default
endpoint: default
status: running
buildkit: 22.06.0-beta.0-902-g2708be0db4.m
platforms: linux/arm64, linux/amd64, linux/amd64/v2, linux/riscv64, linux/ppc64le, linux/s390x, linux/mips64le, linux/mips64
name: desktop-linux
driver: docker
nodes:
name: desktop-linux
endpoint: desktop-linux
status: running
buildkit: 22.06.0-beta.0-902-g2708be0db4.m
platforms: linux/arm64, linux/amd64, linux/amd64/v2, linux/riscv64, linux/ppc64le, linux/s390x, linux/mips64le, linux/mips64 |
|
For I think we should stick to the title-case naming of inspect, and follow the status-quo there. However, I think maybe we could indent the
I think this makes Slightly unrelated, since this has been an issue for a while - how do we determine the size of the table? The produced table is quite large, and now that we're shortening the list of platforms, I was wondering if maybe we could consider setting the default size of the table to the width of the terminal window if that info is accessible? It can be kinda frustrating to view in smaller terminals as is, though it's mostly a separate issue, since it has been like this for a while. |
I think it should be the other way around to be consistent with docker/cli like: $ docker image ls --format raw
repository: buildkit-tests
tag: latest
image_id: 86c59caf767c
created_at: 2022-12-13 07:58:20 +0100 CET
virtual_size: 1.65GB
repository: <none>
tag: <none>
image_id: d57078eda7ec
created_at: 2022-12-13 07:53:45 +0100 CET
virtual_size: 1.65GB
repository: <none>
tag: <none>
image_id: fdc8939962ce
created_at: 2022-12-13 07:47:31 +0100 CET
virtual_size: 1.65GBI want to also add the same flags for the
sgtm for a follow-up as well
Depends if docker/cli provides this, I'm not sure. |
crazy-max
force-pushed
the
ls
branch
2 times, most recently
from
b10b863
to
d949a1d
Compare
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
I'm still a bit on the fence how relevant showing all platforms is in the default presentation?
In general, for the "overview" I (personally) feel like it's "just to get a quick list of builders", so that I can
Within that context, perhaps "native" platform is relevant (I want to use a native |
|
Is there any alternative to this for getting |
Fixes #325
Fixes #380
Fixes #764
Adds
filter,formatandno-truncoptions forlscommand: