remotetool: add --json output #533
Conversation
go/cmd/remotetool/main.go
Outdated
| log.Exitf("error uploading directory for path %s: %v", getPathFlag(), err) | ||
| } | ||
| if *jsonOutput { | ||
| d, _ := json.Marshal(map[string]string{"digest": dg.String()}) |
There was a problem hiding this comment.
How hard would it be to add the cache hit rate and more metadata? In isolated I had included the individual file sizes as a delta-coded integer list. It was really sweet to create an histogram while packing the data as efficiently as possible; https://chromium.googlesource.com/infra/luci/luci-go/+/c364d2ae0a1c94014db11ff581ae7c9fa2ba1e3e/common/isolated/large.go
That said, sums and percentiles are probably sufficient.
There was a problem hiding this comment.
Not too hard -- PTAL.
ola-rozenfeld
force-pushed
the
ola-remotetool-json
branch
from
2fbcafa to
e2f92e2
Compare
go/cmd/remotetool/main.go
Outdated
| us, err := c.UploadDirectory(ctx, getPathFlag()) | ||
| if *jsonOutput { | ||
| js, _ := json.MarshalIndent(us, "", " ") | ||
| fmt.Printf("%s\n", js) |
There was a problem hiding this comment.
Did you mean to write to os.WriteFile(jsonOutput, js, 0o666) ?
There was a problem hiding this comment.
I didn't, but sure, if that's what you prefer :-) Done.
There was a problem hiding this comment.
Sorry to reopen this, but wouldn't printing to stdout accommodate pipes and files (via redirection) while writing to a file complicates the pipe workflow?
There was a problem hiding this comment.
The precedent in the posix way of doing this is to specific "-" as output to stdout. I think we'd get the best of both worlds.
There was a problem hiding this comment.
Isn't that usually for tools that naturally expect files for in/out?
There was a problem hiding this comment.
Users want a tool to be composable. Since remotetool sends its status update to stderr and/or a file, I agree it is possible to use stdout to get the data. While in many case it's great, sometimes the user may want to not take a chance and use a named file instead. For example curl will output to stdout by default but may redirect to the user's choosing with --output and associated flags. Exceptions to that are use case where incrementally of the processing is needed so they do not offer the option to write to a file. A great example is go test -json where the user will want to process a list of json objects.
There was a problem hiding this comment.
To be clear, especially that I commented a lot here, I don't have a strong opinion and I'm fine with whatever @ola-rozenfeld or @mrahs prefers. If you prefer a bool -json flag it's okay too.
There was a problem hiding this comment.
Thanks for taking the time to bounce ideas! No strong opinions here either. json=- (which can be supported later) or -json works for me.
Ola, let me know if you're ready to merge and I'll hit the button.
There was a problem hiding this comment.
Okay, added support for "json=-" for stdout. That's it, should be ready to merge.
Thank you Anas and Marc Antoine for the quick reviews!
Followup to #532
This does the simplest thing possible for now -- we can always add more keys by creating a custom struct that we return from all tool operations, and marshalling that as json.