-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
"Podman image pull" API returns wrong response type and content #10612
Comments
@jwhonce PTAL. This looks like a case where we forgot to update documentation after we added progress reporting? |
The another problem is that API returns
|
@cdoern PTAL |
@sshnaidm Interested in opening a PR to fix? |
Podman returns a 200 even when the image is not found as long as the request itself is valid. 400 is reserved for a bad parameter. I am looking into now why the json is not as it should be |
@sshnaidm I was able to alter the JSON encoding to read as follows:
Before I submit a PR to fix, does this look right to you or is there something more specific you expect to be output here? Also comment above explains 200 vs. 404 response. |
Thanks, I see 400 is reserved, but I'm talking about 404 response. It's logical to have it if no image present, for example like it's done for inspect: https://docs.podman.io/en/latest/_static/api.html#operation/ImageInspectLibpod |
Thanks! That's exactly what I'd expect. |
I'm a little concerned by that change, @cdoern - is it still streaming as the messages occur? It's crucial that the messages be printed as they happen on the server, to provide real-time progress reports to user-facing |
@mheon no what I did was just concatenated all of the stream messages into a string and put the json together when the run context was closed. Should I keep it like this (for final output purposes) and also stream it to the user? I was also able to format it like this using enc.setIndent():
|
It For reference, the current implementation exactly matches Docker, which means we probably did this deliberately. I don't think we should be changing anything here, just documenting how things work. |
@mheon what is the user interactive use case in API? |
@sshnaidm The API also has to support the |
@mheon well... I don't know, having one call without valid JSON in API seems weird to me, even just for progress bar. Maybe adding an option |
If we don't already have a |
ok sounds good @mheon i'll revert to how it was, edit docs and add a quiet param if there isn't one. |
@mheon would something like this for a quiet response:
and then this for a regular response:
be good? |
Should we include the |
Just a note about missing 404 error, the podman-py expects exactly for 404 to raise exception about non pulled image: response = self.client.post("/images/pull", params=params)
response.raise_for_status(not_found=ImageNotFound) |
@sshnaidm The HTTP status code is problematic here. If streaming straight from c/images (as currently written), we write to the client before knowing the final results. @cdoern I do not like seeing all the status messages in one string. That is going to break clients who have built state machines around the expected output stanzas. One implementation, could be to buffer the channel output from c/image in the handler and then present it as expected to the clients and with a better status code. That only burns memory in the service. /cc @mheon |
@jwhonce do you suggest I exclude the |
A friendly reminder that this issue had no activity for 30 days. |
@rhatdan I have the code ready to go to implement what the user wants but I think @jwhonce is concerned about the breakage of current applications depending on the output as is, and the compatibility issues in terms of differing output. This might be a case where, for compatibility we can't change much. I am not sure though, @jwhonce what do you think? |
Does this mean we have to wait for podman 4 for this fix to go in? |
Sure. |
Added quiet param to docs to limit stream output. Formatted JSON. fixes containers#10612 Signed-off-by: cdoern <[email protected]> Signed-off-by: cdoern <[email protected]>
Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)
/kind bug
Description
"Podman image pull" API doesn't return JSON valid response as it should according to documentation.
Steps to reproduce the issue:
curl -XPOST --unix-socket /tmp/podman.sock -v 'http://d/v2.0.0/libpod/images/pull?reference=quay.io%2Fcontainers%2Fpodman'
Describe the results you received:
text instead of JSON response, and even text is not as expected
Describe the results you expected:
According to: https://docs.podman.io/en/latest/_static/api.html#operation/ImagePullLibpod
it should be:
So in our case I'd expect for:
Additional information you deem important (e.g. issue happens only occasionally):
Output of
podman version
:Output of
podman info --debug
:Package info (e.g. output of
rpm -q podman
orapt list podman
):Have you tested with the latest version of Podman and have you checked the Podman Troubleshooting Guide? (https://github.com/containers/podman/blob/master/troubleshooting.md)
No
Additional environment details (AWS, VirtualBox, physical, etc.):
The text was updated successfully, but these errors were encountered: