Fixed issue #4391; podman info --format '{{ json . }}' #4408
Conversation
|
Hi @slimjim2234. Thanks for your PR. I'm waiting for a containers member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
needs-ok-to-test
|
/approve |
|
/ok-to-test |
openshift-ci-robot
added
ok-to-test
and removed
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: slimjim2234, TomSweeneyRedHat The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
cmd/podman/info.go
Outdated
| @@ -2,6 +2,7 @@ package main | |||
|
|
|||
| import ( | |||
| "fmt" | |||
| "strings" | |||
|
@slimjim2234 our ci requires that commits be signed. Please do Although I prefer the style you're using in cmd/podman/info.go for the switch, gofmt doesn't. Please do 'gofmt -s -w cmd/podman/info.go` and readd that, or just remove the added tabs. |
|
Gofmt is not going to pass. I think we are using at least golang 1.12 or 1.13, so your gofmt might be out of date. |
openshift-ci-robot
added
needs-rebase
Signed-off-by: Jimmy Crumpler <[email protected]>
| @@ -88,6 +89,9 @@ func infoCmd(c *cliconfig.InfoValues) error { | |||
|
|
|||
| var out formats.Writer | |||
| infoOutputFormat := c.Format | |||
| if strings.Join(strings.Fields(infoOutputFormat), "") == "{{json.}}" { | |||
There was a problem hiding this comment.
This looks strange, why do you need this strings processesing?
Whis is
if c.Format == "{{json.}}" {
Not enough?
There was a problem hiding this comment.
For the fix I used code from an approved merge found in PR #2688 which solved an nearly identical problem.
If that looks off to you then the other piece of code should change as well.
There was a problem hiding this comment.
i have verified that c.Format == "{{json.}}" does not work.
./bin/podman info --format '{{ json .}}'
Error: template parsing error: template: image:1: function "json" not defined
This fix is more-or-less a band-aid. Would it be worth investigating to determine the larger issue?
There was a problem hiding this comment.
@baude Why was this necessary? Is this just some because the c.Format is a StirngVarP? Versus a StringVar?
There was a problem hiding this comment.
This is special casing to support a strange Docker convention where --format '{{ json. }}' is the same as --format json.
There was a problem hiding this comment.
The reason for strings.Fields is whitespace removal.
|
could you add a test? |
What kind of test? I have committed a small and simple test validating the "{{ json . }}" format usage. |
|
@slimjim2234 Thanks |
|
The logic for detecting whether a template is in use looks a bit weird to
me - hold off merging for a bit so I can take a closer look.
…On Sun, Nov 3, 2019, 05:40 Daniel J Walsh ***@***.***> wrote:
@slimjim2234 <https://github.com/slimjim2234> Thanks
Other then the question to @baude <https://github.com/baude> LGTM
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#4408>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3AOCHHJYKJ5BAZYIHVKETQR2MBLANCNFSM4JHVODHA>
.
|
|
@mheon Any progress? @slimjim2234 Any chance of getting a test? |
|
Code LGTM |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
JSON Go template format now recognized through the info action.
Fixes issue #4391