Fixup search #9070
Fixup search #9070
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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
pkg/api/handlers/libpod/images.go
Outdated
| if err != nil { | ||
| utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, errors.Wrapf(err, "failed to parse %q header for %s", key, r.URL.String())) | ||
| utils.Error(w, "Something went wrong.", http.StatusNotFound, err) |
There was a problem hiding this comment.
We need to distinguish between Not Found and Internal Server Error here.
There was a problem hiding this comment.
NotFound is not an error, it just returns nothing.
$ ./bin/podman search registry.access.redhat.com/danwalsh
$ ./bin/podman-remote search registry.access.redhat.com/danwalsh
$
There was a problem hiding this comment.
If NotFound is not an error, why are we returning it here? Docker API indicates the correct code is 500 - https://docs.docker.com/engine/api/v1.40/#operation/ImageSearch
There was a problem hiding this comment.
Well podman search imagenotfound
Does not return an error.
I don't think we should do podman search imagenotfound to return an error.
But if we have to, then I will need to do this for only compat endpoint.
rhatdan
force-pushed
the
search
branch
4 times, most recently
from
712c01e to
c40b050
Compare
|
i personally do not like the idea of combining the podman and compat endpoints when they have differently supported variables. I think it is asking for problems in the future. that said, lgtm |
| if err != nil { | ||
| utils.BadRequest(w, "term", query.Term, err) | ||
| utils.Error(w, "Something went wrong.", http.StatusBadRequest, err) |
There was a problem hiding this comment.
This should be StatusInternalServerError
There was a problem hiding this comment.
A Bad Parameters is handled above, line 33. This is a more general "things have gone wrong" error which is a 500.
These error codes are the best error handling we have in the HTTP API so we need to get them right.
podman-remote search had some FIXMEs in tests that were failing. So I reworked the search handler to use the local abi. This means the podman search and podman-remote search will use the same functions. While doing this, I noticed we were just outputing errors via logrus.Error rather then returning them, which works ok for podman but the messages get lost on podman-remote. Changed the code to actually return the error messages to the caller. This allows us to turn on the remaining podman-remote FIXME tests. Signed-off-by: Daniel J Walsh <[email protected]>
|
/lgtm |
|
I'm coming back to this as I write the release notes for v3.1.0-RC2, and I notice that we did not document the new options added to the handler in Swagger. The same can be said of the other reworks Dan did this patch cycle. What's the thinking on these - are they internal-only and undocumented otherwise, or do we need to go back and find everything added and add docs? @jwhonce @baude @rhatdan |
|
These are internal and bug fixes. |
github-actions
bot
added
the
locked - please file new issue/PR
podman-remote search had some FIXMEs in tests that were failing.
So I reworked the search handler to use the local abi. This
means the podman search and podman-remote search will use the
same functions.
While doing this, I noticed we were just outputing errors via
logrus.Error rather then returning them, which works ok for
podman but the messages get lost on podman-remote. Changed
the code to actually return the error messages to the caller.
This allows us to turn on the remaining podman-remote FIXME
tests.
Signed-off-by: Daniel J Walsh [email protected]