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

builder: implement PullParent option with buildkit #37613

Merged
merged 1 commit into from Aug 17, 2018

Conversation

Projects
None yet
5 participants
@tiborvass
Copy link
Collaborator

tiborvass commented Aug 8, 2018

Signed-off-by: Tibor Vass tibor@docker.com

This allows to run docker build --pull ... when buildkit is enabled.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 14, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@b6242da). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #37613   +/-   ##
=========================================
  Coverage          ?   35.85%           
=========================================
  Files             ?      606           
  Lines             ?    44704           
  Branches          ?        0           
=========================================
  Hits              ?    16029           
  Misses            ?    26466           
  Partials          ?     2209
@vdemeester
Copy link
Member

vdemeester left a comment

LGTM 🐯

@thaJeztah
Copy link
Member

thaJeztah left a comment

SGTM

ping @tonistiigi PTAL

@@ -115,11 +115,17 @@ func (is *imageSource) resolveLocal(refStr string) ([]byte, error) {
}

func (is *imageSource) ResolveImageConfig(ctx context.Context, ref string, opt gw.ResolveImageConfigOpt) (digest.Digest, []byte, error) {
if preferLocal {

This comment has been minimized.

@thaJeztah

thaJeztah Aug 14, 2018

Member

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)

if preferLocal {
dt, err := p.is.resolveLocal(p.src.Reference.String())
if err == nil {
p.config = dt
}
}

@tiborvass tiborvass force-pushed the tiborvass:buildkit-pull branch 2 times, most recently from 85b7c63 to cfa31c9 Aug 17, 2018

@tiborvass

This comment has been minimized.

Copy link
Collaborator Author

tiborvass commented Aug 17, 2018

@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 --pull is the same between legacy and buildkit builders.

@vdemeester
Copy link
Member

vdemeester left a comment

Still LGTM 😉

builder: implement PullParent option with buildkit
Signed-off-by: Tibor Vass <tibor@docker.com>

@tiborvass tiborvass force-pushed the tiborvass:buildkit-pull branch from cfa31c9 to 7c1c8f1 Aug 17, 2018

@tiborvass

This comment has been minimized.

Copy link
Collaborator Author

tiborvass commented Aug 17, 2018

Just removed preferLocal constant.

@thaJeztah
Copy link
Member

thaJeztah left a 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

This comment has been minimized.

@thaJeztah

thaJeztah Aug 17, 2018

Member

nit; could be

return is.resolveRemote(ctx, ref, opt.Platform)

This comment has been minimized.

@tiborvass

tiborvass Aug 17, 2018

Author Collaborator

@thaJeztah true, this was because the original (desired) code is the one commented out below in which case the assignments do make sense.

@@ -209,6 +209,12 @@ func (b *Builder) Build(ctx context.Context, opt backend.BuildConfig) (*builder.
frontendAttrs["no-cache"] = ""
}

if opt.Options.PullParent {
frontendAttrs["image-resolve-mode"] = "pull"

This comment has been minimized.

@thaJeztah

thaJeztah Aug 17, 2018

Member

Thinking if it would be good to use the values that are / will be defined in source.ResolveMode

This comment has been minimized.

@tiborvass

tiborvass Aug 17, 2018

Author Collaborator

@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.

This comment has been minimized.

@tiborvass

This comment has been minimized.

@thaJeztah

thaJeztah Aug 17, 2018

Member

Yup; definitely not a blocker (same for my other comment) 👍

@thaJeztah thaJeztah merged commit ba5c92f into moby:master Aug 17, 2018

8 checks passed

codecov/patch Coverage not affected.
Details
codecov/project No report found to compare against
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 41672 has succeeded
Details
janky Jenkins build Docker-PRs 50443 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 10869 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 21752 has succeeded
Details
z Jenkins build Docker-PRs-s390x 10737 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.