compat: add layer caching compatiblity for non-podman clients.
#12381
Conversation
|
@stac47 Could you please try this with |
flouthoc
force-pushed
the
build-layer-docker-compat
branch
2 times, most recently
from
584ad53 to
0ba5007
Compare
flouthoc
force-pushed
the
build-layer-docker-compat
branch
from
0ba5007 to
c05d8c2
Compare
|
Hi, |
|
@flouthoc, it looks like tests break. I did not check but suspect that the libpod endpoint is calling the same handler, so we may need to make setting the default conditional (i.e., do not set for libpod calls). |
|
LGTM once tests go green, and concur that @vrothberg is on the right track on the cause of the failures |
flouthoc
force-pushed
the
build-layer-docker-compat
branch
from
c05d8c2 to
6fed507
Compare
|
@vrothberg ackd then i guess previous version of commit should handle this, which only sets it |
| // if layers field not set assume its not from a valid podman-client | ||
| // could be a docker client, set `layers=true` since that is the default | ||
| // expected behviour | ||
| if _, found := r.URL.Query()["layers"]; !found { |
There was a problem hiding this comment.
That's not enough. We need to check whether the handler was called via libpod or compat. Otherwise we depend on behavior that may change.
Wrap that into if !utils.IsLibpodRequest(r) { ..}
There was a problem hiding this comment.
Why should layers not default to true in the libpod API? podman build defaults to true so should the API IMO.
There was a problem hiding this comment.
@Luap99 i guess because podman-remote has a option to configure it as false so we must honor it.
There was a problem hiding this comment.
We will always honor it, this is just the default.
There was a problem hiding this comment.
@vrothberg @mheon Why should the API have a different default then the podman cli?
There was a problem hiding this comment.
Tests broke so there is an expectation on these libpod defaults. Changing them seems like a separate issue to me.
|
@flouthoc Can you please document that parameter in the swagger doc. It looks like |
Non-podman clients do not set `layers` while making request. This is supposed to be `true` bydefault but `non-podman-clients i.e Docker` dont know about this field as a result they end up setting this values to `false`. Causing builds to never use cache for layers. Adds compatiblity for `docker SDK`. [NO NEW TESTS NEEDED] Signed-off-by: Aditya Rajan <arajan@redhat.com>
Add missing `layer` entry to swagger docs for `/build`. Signed-off-by: Aditya Rajan <arajan@redhat.com>
flouthoc
force-pushed
the
build-layer-docker-compat
branch
from
6fed507 to
13ee178
Compare
|
/lgtm |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, 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
bot
added
the
approved
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
Non podman clients do not set
layerswhile making request. This issupposed to be
trueby default butnon-podman-clients i.e Dockerdontknow about this field as a result they end up setting this value to
false. Causing builds to never use cache for layers.Adds compatibility for
non-podman (docker SDK).[NO NEW TESTS NEEDED]
Cant find any convenient way to add a new test for this via podman client.
Closes: #12378