builder: implement PullParent option with buildkit#37613
builder: implement PullParent option with buildkit#37613thaJeztah merged 1 commit intomoby:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #37613 +/- ##
=========================================
Coverage ? 35.85%
=========================================
Files ? 606
Lines ? 44704
Branches ? 0
=========================================
Hits ? 16029
Misses ? 26466
Partials ? 2209 |
There was a problem hiding this comment.
Noticed that puller.resolveLocal() still uses this constant; looking at where that's called that seems to be only used to determine if an image is in the cache, which likely isn't done until after this function (ResolveImageConfig()) has been called? Just wondering if that part of the code will also be configurable, or will be the only behavior (in which case we can remove the preferLocal constant)
moby/builder/builder-next/adapters/containerimage/pull.go
Lines 216 to 221 in 4122eb1
tiborvass
force-pushed
the
buildkit-pull
branch
2 times, most recently
from
85b7c63 to
cfa31c9
Compare
|
@vdemeester @thaJeztah I updated this. I fixed an issue whereby "local" would error out if local was not found, when in fact it just means "prefer" local, but still fallback to remote if cannot find. Ideally I'd like "pull" to also fallback to local (like when you're on the plane and don't have network). Essentially there is no difference between "default" and "local", other than I use "default" so that in the future we may improve on the UX if we want to. Either way, right now, the behavior of |
Signed-off-by: Tibor Vass <[email protected]>
tiborvass
force-pushed
the
buildkit-pull
branch
from
cfa31c9 to
7c1c8f1
Compare
|
Just removed |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
triggered CI to do another run
| dgst, dt, err := is.resolveRemote(ctx, ref, opt.Platform) | ||
| // TODO: pull should fallback to local in case of failure to allow offline behavior | ||
| // the fallback doesn't work currently | ||
| return dgst, dt, err |
There was a problem hiding this comment.
nit; could be
return is.resolveRemote(ctx, ref, opt.Platform)There was a problem hiding this comment.
@thaJeztah true, this was because the original (desired) code is the one commented out below in which case the assignments do make sense.
| } | ||
|
|
||
| if opt.Options.PullParent { | ||
| frontendAttrs["image-resolve-mode"] = "pull" |
There was a problem hiding this comment.
Thinking if it would be good to use the values that are / will be defined in source.ResolveMode
There was a problem hiding this comment.
@thaJeztah actually, could do it in a follow up once buildkit has a String() method on it. I don't want to rely on llb package at this level of abstraction.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yup; definitely not a blocker (same for my other comment) 👍
5 participants
Signed-off-by: Tibor Vass [email protected]
This allows to run
docker build --pull ...when buildkit is enabled.