-
Notifications
You must be signed in to change notification settings - Fork 471
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: add format, quiet and no-trunc opts #830
Draft
crazy-max
wants to merge
1
commit into
docker:master
Choose a base branch
from
crazy-max:ls
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+13,233
−64
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,7 +2,10 @@ package builder | |||||
|
||||||
import ( | ||||||
"context" | ||||||
"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" | ||||||
|
@@ -163,6 +166,45 @@ func (b *Builder) LoadNodes(ctx context.Context, withData bool) (_ []Node, err e | |||||
return b.nodes, nil | ||||||
} | ||||||
|
||||||
func (n *Node) MarshalJSON() ([]byte, error) { | ||||||
var status string | ||||||
if n.DriverInfo != nil { | ||||||
status = n.DriverInfo.Status.String() | ||||||
} | ||||||
var err string | ||||||
if n.Err != nil { | ||||||
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"` | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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;
Suggested change
|
||||||
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, | ||||||
Flags: n.Flags, | ||||||
DriverOpts: n.DriverOpts, | ||||||
Files: n.Files, | ||||||
Status: status, | ||||||
ProxyConfig: n.ProxyConfig, | ||||||
Version: n.Version, | ||||||
Err: err, | ||||||
}) | ||||||
} | ||||||
|
||||||
func (n *Node) loadData(ctx context.Context) error { | ||||||
if n.Driver == nil { | ||||||
return nil | ||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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⚠️ ), the output seems to look fine.
platforms.Format()
? Out of curiousity, I tried what happened if we use[]ocispecs.Platform
as-is, and (at a glanceThis is still using the ad-hoc type, but updating it to use OCI platform;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the above, the output seems to be fine (but perhaps there's specific corner-cases we're not accounting for?)
👆 😂 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤔)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and I guess it doesn't show the
default
/native
platform; (linux/arm64*
vslinux/arm64
);edit:⚠️ actually, that's not it (where is that added?)
If we really want a string presentation in all cases, we could have a local type that wraps
oci.Platform
and implementsMarshalText
;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for the
--format json
so we are consistent with table / raw result.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.
*
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?).