handle json formatting for print#2374
Conversation
Signed-off-by: Tonis Tiigi <[email protected]>
| return printValue(subrequests.PrintDescribe, subrequests.SubrequestsDescribeDefinition.Version, f.Format, res) | ||
| default: | ||
| if dt, ok := res["result.txt"]; ok { | ||
| if dt, ok := res["result.json"]; ok && f.Format == "json" { |
There was a problem hiding this comment.
(Sorry, reading from my phone, so wasn't able to check related code easily) is f.Format here the value specified through one of the command-line flags?
If so, should the "else" condition produce an error (as we're unable to satisfy the format)?
Thinking of situations where the user may try to parse JSON output and now ending up with a different format
There was a problem hiding this comment.
This adds "json" handling for the default: case of the switch where previously it was only handled by the case: branches. If the frontend does not provide expected output that contains info about how buildx can print it then the behavior atm is not well defined. Currently, in that case it prints both the full request (with expected format) and the full result from the frontend(instead of a specific known key). Maybe it should indeed error when user specifically asked for json, but that looks outside of the scope of the current PR.
There was a problem hiding this comment.
Actually, looking at printValue, for known keys if frontend says it supports it but then does not provide JSON value it just prints empty value (that would not be valid json either). So it is already inconsistent. I could make the current new branch return empty value as well but that doesn't look like something that would help the user.
There was a problem hiding this comment.
Thanks! Yes, looks like there's possibly some improvements to make to make it better defined. Very likely not a blocker for this PR though.
Should we create a tracking ticket for that effort?
4 participants
JSON formatting was handled by the known print names, but unclear why the default case was not handled as well if user requested JSON output.
@daghack