With Buildkit, don't pull docker images needlessly.

[neil-ach]

When using Docker with Buildkit and inline cache information enabled,
docker build will pull image layers to use as cache on demand.
As a result, unconditionally pulling the cache is both superfluous and
slower, so let's not do that.

[bradrydzewski]

could we remove the above for loop, and instead move this logic into a helper function? I'm having a bit of trouble understanding the if statement

if useCacheFrom(p) {
  ...
} else {
  ...
}

func useCacheFrom(p ...) bool {
	// if buildkit inline cache, return true
	for _, v := range p.Build.Args {
		if v == "BUILDKIT_INLINE_CACHE=1" {
			return true
		}
	}
	// else if buildkit, return false
	if os.Getenv("DOCKER_BUILDKIT") == "1" {
		return false
	}
	// else if not buildkit, return true
	return true
}

also might be good to comment the logic so folks unfamiliar with buildkit can follow the code. thanks!!

[neil-ach]

Buildkit (when available and enabled) uses the inline cache information to pull only the relevant subset of cache, which is in general more efficient than unconditionally pulling the entire image in the hopes that some of it will trigger cache hits.

For the above to work, we need Buildkit enabled (DOCKER_BUILDKIT=1) AND Buildkit inline cache enabled (BUILDKIT_INLINE_CACHE=1) AND cache-from set to a valid image (with inline cache metadata present).

However, right now if this plugin sees the cache-from argument set, it will add the pre-pull of the image step, and that makes the more efficient Buildkit inline cache useless, because all of the cache is already fetched, useful or not. This is the part that we want to change. We never want to remove the cache-from directive if it was passed, we want to skip the image pre-pull when we know that it will be counterproductive (the above outlined scenario).

I don't mind adding some more documentation, although this PR combined with the commit message should serve as pretty in depth documentation at this point.

Thanks.

[bradrydzewski]

thanks for the explanation. let's split the code into a helper function similar to the pseudo-code in my comment, with any changes that you feel are needed to make it accurate, and then we can get this merged

[bradrydzewski]

thanks for the explanation. let's split the code into a helper function similar to the pseudo-code in my comment, with any changes that you feel are needed to make it accurate, and then we can get this merged

[neil-ach]

I would, but the suggested changes do not produce the desired behaviour. There is no situation where removing the cache-from argument does what we want, and that is what the suggested changes do. Please let me know if there's something from my previous comment that isn't clear about this, or if you have a different suggestion.

[bradrydzewski]

the code in my comment is pseudo code, so please feel free to adjust the logic as needed. The request is to break out into a helper function to improve readability (bonus points for adding a unit test) to help make the code easier to understand.

[neil-ach]

Sure, I can move the pre-pull determination into a helper function.