Expose functionality to download buildpacks

[YousefHaggyHeroku]

Summary

This PR aims to address #761 by introducing a new DownloadBuildpack function. DownloadBuildpack allows consumers of pack as a library to use the existing buildpack URI parsing, downloading, and extracting logic without having to reimplement most of the work. One requirement for this request is to give consumers the option to not use docker; the existing imageFetcher struct seems to handle this concern.

Q: For this change, I also moved blob and image to become public facing packages so that consuming libraries can pass in their own imageFetchers and downloaders. Would it be simpler and more preferable to just keep those internal and create an imageFetcher if one isn't found? Something like:

if opts.imageFetcher == nil{
imageFetcher := image.NewFetcher(...)
}else{
imageFetcher := opts.ImageFetcher
}

Same question for the downloader. We could also both make the packages public and add that default creation logic to make life easier for consumers that don't want a custom fetcher while still keeping flexibility for those that do

Output

Before

After

Documentation

• Should this change be documented?
  • Yes, see #___
  • No

Related

Resolves #761

[aemengo]

Thanks so much for this contribution!

Is it for your use-case that this DownloadBuildpack( method doesn't have a receiver on the Client struct? To me, it looks a little awkward passing such hefty abstractions through the opts parameter like this. Furthermore, it would address the following question you raised, since the func NewClient( factory method will in fact initialize an ImageFetcher and Downloader if they don't exist. Finally, it would avoid awkward lines like this.

Would it be simpler and more preferable to just keep those internal and create an imageFetcher if one isn't found?

I'd prefer to limit the scope of this PR and not expose those files (downloader.go, fetcher.go, etc), until someone requires that level of configurability.

Lastly, I think some tests are warranted for this method. Since this is now a public facing interface that anyone can use, and reputations are on the line. 😬

[YousefHaggyHeroku]

Thank you for your feedback! The only issue with having a receiver on Client is that the NewClient factory method creates a few unnecessary things for our use case like a docker client and a lifecycle executor. One requirement for our use case is to avoid using docker entirely.

One I idea I had was to create a new Factory method, NewSimpleClient or perhaps NewDockerlessClient, that doesn't force create a docker client. What are your thoughts on this approach?

Good point about tests, I can definitely go through and add some once we are happy with the opts and the format of this method.

[aemengo]

While I normally wouldn't think twice about NewSimpleClient or NewDockerlessClient in more "private" code, I'd prefer not to introduce a different code path so early in a public facing interface.

Our docker client is configurable... What are your thoughts on the following? Just something that will satisfy the interface but won't actually do anything:

// WithOutDockerClient supply a no-op docker client.
func WithOutDockerClient() ClientOption {
	return func(c *Client) {
		c.docker = &noopDockerClient{}
	}
}

🤔 I'm a little torn with having to create/generate a dockerClient.CommonAPIClient replacement ourselves. For the lifecycleExecutor, I have less reservations. I'm also completely for your client-side code passing in these no-op types.

Happy to hear others' thoughts on this!

[YousefHaggyHeroku]

Ah interesting, I hadn't thought of passing a no-op docker client in. In that case, I feel like the existing WithDockerClient option suffices, and we can just pass in a no-op docker client in our consuming code. With your explanation, no changes to NewClient and no new ClientOption makes the most sense to me, unless anyone else has other thoughts here.

[YousefHaggyHeroku]

I've made the discussed change to take add a Client receiver and moved blob back into internal. I kept image public so that we can at-least pass in image.FetchOptions. If we don't want the entire image package to be public, I could split it into an internal package and external package with the types, but that doesn't sound ideal to me. Are there any strong reasons why we shouldn't just make the image package public now?

[aemengo]

No strong reasons, from me. 🙂

[jromero]

If image.FetchOptions is the only reason for the move, I would suggest that we try to find alternatives. The Fetcher would otherwise become a public interface that we'd need to be considerate about making changes to in the future.

[YousefHaggyHeroku]

2 updates.

• Added tests. Most of which are extracted from the existing CreateBuilder tests. I had to refactor things a bit to be more testable
• I've moved image back to internal, instead just directly defining Daemon and PullPolicy fields.

It probably seems odd that mock_buildpack_downloader.go is not in testmocks, but I can't put it into testmocks bc of a circular dependency issue. Some of the test files are under the package pack, which requires testmocks which would require pack. Two probably neater approaches to fixing this so that the mock could be in testmocks:

• The test files defined with package pack should instead be under package pack_test. This is actually a lot of work
• Move buildpack_downloader to a new package. Also a small chunk of work: it currently uses ~3 private pack functions that are all in different files.

Those solutions might be more trouble than they are worth, im not sure 🤷

[jromero]

@YousefHaggyHeroku This looks great! There's a conflict now also need to get sign-offs on some of the commits. 😞

[YousefHaggyHeroku]

@YousefHaggyHeroku This looks great! There's a conflict now also need to get sign-offs on some of the commits. 😞

Just handled both, thanks!