Skip to content
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

pkg/report: fix IsJSON() #1226

Merged
merged 1 commit into from
Nov 8, 2022
Merged

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Nov 8, 2022

When a user request --format {{json .}} they would want the go template parser to handle it. Currently we overwrite this and assume that {{json .}} equals json. This is not correct. When the output is a range (array), i.e. podman ps, it should return one json object per line and not a json array which is the case with json.

This is required for docker compat.

Fixes containers/podman#16436

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 8, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Nov 8, 2022
When a user request --format `{{json .}}` they would want the go
template parser to handle it. Currently we overwrite this and assume
that `{{json .}}` equals `json`. This is not correct. When the output is
a range (array), i.e. podman ps, it should return one json object per
line and not a json array which is the case with `json`.

This is required for docker compat.

Fixes containers/podman#16436

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99
Copy link
Member Author

Luap99 commented Nov 8, 2022

/hold
I should open a PR in podman with this fix first to make sure all test still pass.

@Luap99
Copy link
Member Author

Luap99 commented Nov 8, 2022

containers/podman#16446 for podman tests

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rhatdan
Copy link
Member

rhatdan commented Nov 8, 2022

/lgtm
/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit bf9e18b into containers:main Nov 8, 2022
@Luap99
Copy link
Member Author

Luap99 commented Nov 8, 2022

wait, the podman PR has red test! This will break vendoring.
I will send another PR which hopefully fixes it.

@Luap99 Luap99 deleted the format-json branch November 8, 2022 15:37
Luap99 added a commit to Luap99/common that referenced this pull request Nov 8, 2022
The PR containers#1226 was merged to soon, it breaks podman tests and backwards
compat. `{{json}}` is not a valid template but it worked before the same
as `json` so we should keep that.

Fixes up commit 152c840

Signed-off-by: Paul Holzinger <[email protected]>
Luap99 added a commit to Luap99/common that referenced this pull request Nov 8, 2022
The PR containers#1226 was merged to soon, it breaks podman tests and backwards
compat. `{{json}}` and `{{json.}}` are not valid templates but it worked
before the same as `json` so we should keep that for compat reasons.

Fixes up commit 152c840

Signed-off-by: Paul Holzinger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling inspect with a {{json .}} template doesn't unwrap the object
4 participants