Add asset package creation #1085

dwillist · 2021-02-19T05:55:07Z

Signed-off-by: dwillist [email protected]

Summary

Implement asset-cache creation as specified by #1055

This adds the experimental create-asset-cache command that works as follows:
pack create-asset-cache <image-name> --buildpack <buildpack locator> --publish(optional) --pull-policy(optional)

Additionally it adds support for using asset caches when

Running pack build with the --asset-cache flag
Adding asset caches to builders.

~~(side UI bug, local buildpack require a file:// prefix...~~

Documentation

Should this change be documented?
- Yes, (whole feature to be documented upon completion)
- No

Codecov Report

Merging #1085 (89afd7a) into main (7a08937) will decrease coverage by 0.43%.
The diff coverage is 79.30%.

❗ Current head 89afd7a differs from pull request most recent head 46bba8f. Consider uploading reports for the commit 46bba8f to get more accurate results

@@            Coverage Diff             @@
##             main    #1085      +/-   ##
==========================================
- Coverage   80.97%   80.55%   -0.42%     
==========================================
  Files         136      155      +19     
  Lines        8305     9219     +914     
==========================================
+ Hits         6724     7425     +701     
- Misses       1156     1341     +185     
- Partials      425      453      +28

Flag	Coverage Δ
os_linux	`80.02% <79.55%> (-0.51%)`	⬇️
os_macos	`77.87% <79.55%> (-0.20%)`	⬇️
os_windows	`80.47% <79.30%> (-0.41%)`	⬇️
unit	`80.09% <79.55%> (-0.47%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

acceptance/acceptance_test.go

create_asset_cache.go

ekcasey · 2021-02-24T15:35:51Z

@dwillist I wonder if this feature should only be supported for buildpack API 0.7+ (which doesn't exist yet). Older buildpack APIs don't support assets in buildpack.toml.

buildpacks/spec#190

jromero · 2021-03-11T15:08:11Z

Given that this feature is tied to a spec change, we need to take special precautions.

Options:

Gate this feature behind experimental. This would allow us to add the feature in "chunks", providing the feature to users while at the same time portraying that the implementation may change based on spec changes.
Don't merge set of changes related to this feature until the spec changes have been merged.

EDIT: provided more context/clarity

dwillist · 2021-04-26T04:33:42Z

PR overview

As things stand I think this is ready for PR review, I'll be updating this comment with additional acceptance testing materials for folks if the want to use them.

Whats implemented

Note that all of the below are hidden behind experimental (even the new pack build ... flags)

Add a pack asset-package create command
- this command takes list of buildpacks and creates an asset cache image as an image or tarball.
- TODO: gate the creation of packages based on a buildpack API version. We cannot really test this until we have a lifecycle that supports the new API.
Add the ability to add assets to a builder by specifying them in the builder.toml configuration file. (Add an example file here)
- TODO gate the creation of builders with assets on the platform API version. Again we need to have the lifecycle release to get the actual platform API we should be gating on.
Add the ability to add assets to a pack build
- again gate this ability based on the platform API that will be used by pack & the underlying builder.

Whats left to do.

Once there is a lifecycle release that solidifies the platform API and buildpack API needed to use assets we can gate the usage of asset caches around the following support table:

pack used to build supports platform api >= 0.7	pack used to create builder supports platform api >= 0.7	lifecycle supports platform api >= 0.7	conclusion
T	T	T	CNB_ASSETS can be set in build env! and buildpacks can use it
T	T	F	CNB_ASSETS cannot be used as lifecycle does not support API Do not allow assets to be added to builders like this.
T	F	T	CNB_ASSETS will not be set on the builder :( and assets were not allowed on the builder when it was created. But we should support adding assets using: "pack build ... --asset-package" at build time by adding CNB_ASSETS to the environment
T	F	F	CNB_ASSETS cannot be used as lifecycle does not support API...
F	T	T	Cannot use assets as pack used to build does not support platform api 0.7
F	T	F	CNB_ASSETS cannot be used as lifecycle does not support API...
F	F	T	Cannot use assets as pack used to build does not support platform api 0.7
F	F	F	CNB_ASSETS cannot be used as lifecycle does not support API...

dwillist · 2021-05-20T18:33:13Z

@dfreilich any chance you could take a look at this? I know its pretty large but would be nice to try and get this into the next release if possible.

dfreilich

This is only a partial review - I still have to check over the 2nd half of the files.

Largely, this is super super impressive work. I haven't validated that it works, but architecturally and cleanliness, this is really 🤩 .

One thing I'm not a fan of at the moment is the command name - maybe it's overkill, but I'd really rather it be pack asset create, rather than create-asset..., to fit with our general pattern (and it opens the door for us to make our architecture a bit more pluggable, once all of the commands follow the same general pattern). Great work though!

acceptance/acceptance_test.go

dfreilich · 2021-05-20T19:01:00Z

acceptance/acceptance_test.go

+							os.RemoveAll(tmpDir)
+							imageManager.CleanupImages(repoName)
+						})
+						it("adds assets to build and allows buildpacks to access them", func() {


Same complaint as the first test - this is very heavy, and it also follows a similar path in preparation as the first test. Try to refactor this, preferrably into a dedicated AssetPackagesManager or XBuildpack.

dfreilich · 2021-05-20T19:02:40Z

acceptance/acceptance_test.go

@@ -2621,6 +3019,137 @@ func createComplexBuilder(t *testing.T,
 	return bldr, nil
 }

+func createBuilderWithAssets(


When possible, I'd rather we err towards taking things out of this file, rather than into this acceptance test. I'm not sure if this is the place to do it, but we really should have a BuilderManager (as I mentioned here #673 (comment)) to take care of builder prep.

dfreilich · 2021-05-20T19:03:37Z

acceptance/acceptance_test.go

+func cleanAbsPath(path string) string {
+	return strings.ReplaceAll(path, `\`, `\\`)
+}


Can we put this in a more central place? testutils, perhaps?

dfreilich · 2021-05-20T19:04:06Z

acceptance/acceptance_test.go

+	return strings.ReplaceAll(path, `\`, `\\`)
+}
+
+func imageSha(t *testing.T, assert h.AssertionManager, dockerCli client.CommonAPIClient, repoName string) string {


This should go into the imageManager or ContainerManager. It may already exist, honestly

dfreilich · 2021-05-20T19:28:39Z

create_asset_package.go

+
+// CreateAssetPackage writes a new Asset Package image using options specified in opts.
+// This image can be used to add assets to builds.
+func (c *Client) CreateAssetPackage(ctx context.Context, opts CreateAssetPackageOptions) error {


Overall, this func reads really well. It's decomposed super nicely. Great job!

dfreilich · 2021-05-20T19:30:08Z

create_asset_package.go

+	return result
+}
+
+// this method mutates the given assetImg


This comment doesn't really add much

dfreilich · 2021-05-20T19:30:51Z

create_asset_package_test.go

+func assertLayerHasFileWithContents(t *testing.T, rc io.ReadCloser, path, expectedContents string) {
+	t.Helper()
+
+	_, actualContents, err := archive.ReadTarEntry(rc, path)
+	h.AssertNil(t, err)
+	h.AssertEq(t, string(actualContents), expectedContents)
+}


Can we add this to testutils, in case it's useful in other tests?

dfreilich · 2021-05-20T19:33:33Z

internal/asset/fetcher.go

+// ImageFetcher is implemented by Fetcher which allows work with remote and local images,
+// as well as control when images are used locally vs pulled remotely.
+type ImageFetcher interface {
+


Suggested change

nit

dfreilich · 2021-05-20T19:35:58Z

internal/asset/locator.go

+}
+
+// GetLocatorType parses a locator and returns the LocatorType
+func GetLocatorType(locator string, relativeBaseDir string) LocatorType {


Is there some way we can combine this with the existing locator searching mechanism, or is it totally unfit/unintuitive to do so?

Definitly could but it would likely take quite a bit of work here to pull out all the buildpack locator specific logic. Was thinking this would be part of the larger refactoring story.

dfreilich · 2021-05-20T19:40:30Z

@dwillist I was waiting until the conflicts were resolved, because changes will have to be made, but I made an initial pass. I'll make another pass at some point soonm, to give some more detailed feedback on the 2nd half of the PR.

Signed-off-by: dwillist <[email protected]>

dfreilich

I've checked it over (not quite as in depth as I would like) and it definitely seems to be good.

To be honest, I don't have the time allocated to the project right now to give it the in depth review it really needs (on the scale of 4+ hours, I would assume), and its current iteration as one massive PR isn't the most conducive for it being a comprehensively reviewed PR, unfortunately. At the same time, since it's gated with an experimental flag, I'm ok merging it in, assuming that there will be further refactors and simplifications/combinations (for instance, seeing whether we can combine some of the primitives which seem to be redefined, such as the separate Fetchers, locators, etc).

Given that (and how long it's been waiting), I'll give it a approval now, but would definitely appreciate smaller PRs in the future 😅

dfreilich · 2021-06-15T18:44:40Z

@dwillist Looks like there's some issues with acceptance tests which needs to be fixed before merging

jromero · 2021-10-06T16:25:10Z

Based on efforts to unify caching (see buildpacks/rfcs#171) the original RFC has been put on Hold. Closing this PR (at least for now).

dwillist requested a review from a team as a code owner February 19, 2021 05:55

github-actions bot added this to the 0.18.0 milestone Feb 19, 2021

github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Feb 19, 2021

dwillist marked this pull request as draft February 19, 2021 15:02

dwillist force-pushed the asset_cache branch 2 times, most recently from 9966d9f to 6d67a28 Compare February 19, 2021 17:12

aemengo reviewed Feb 19, 2021

View reviewed changes

internal/blob/downloader.go Show resolved Hide resolved

aemengo reviewed Feb 19, 2021

View reviewed changes

build.go Outdated Show resolved Hide resolved

dwillist force-pushed the asset_cache branch 2 times, most recently from 1bb5ff1 to fcd8a86 Compare February 19, 2021 20:03

dwillist force-pushed the asset_cache branch from 335ad43 to 9f93ecd Compare February 22, 2021 19:13

github-actions bot added the type/chore Issue that requests non-user facing changes. label Feb 23, 2021

dwillist force-pushed the asset_cache branch 2 times, most recently from 179da37 to f109c75 Compare February 23, 2021 07:23

dwillist commented Feb 23, 2021

View reviewed changes

acceptance/acceptance_test.go Outdated Show resolved Hide resolved

dwillist commented Feb 23, 2021

View reviewed changes

create_asset_cache.go Outdated Show resolved Hide resolved

dwillist force-pushed the asset_cache branch 4 times, most recently from 3b7ebba to 55d88f3 Compare February 23, 2021 22:26

aemengo reviewed Feb 23, 2021

View reviewed changes

create_asset_cache.go Outdated Show resolved Hide resolved

dwillist force-pushed the asset_cache branch 2 times, most recently from f42f95b to 931f688 Compare March 1, 2021 19:26

dwillist marked this pull request as ready for review March 3, 2021 16:11

jromero added status/blocked Issue or PR that is blocked. See comments. and removed type/chore Issue that requests non-user facing changes. labels Mar 11, 2021

dwillist force-pushed the asset_cache branch from 0ee0311 to 8084466 Compare April 22, 2021 15:16

Merge branch 'main' into asset_cache

89afd7a

dwillist marked this pull request as ready for review April 26, 2021 04:34

dwillist added size/lg Large level of effort and removed status/blocked Issue or PR that is blocked. See comments. type/chore Issue that requests non-user facing changes. labels May 4, 2021

dwillist mentioned this pull request May 12, 2021

add asset packages to distribution & buildpack spec buildpacks/spec#201

Closed

dfreilich requested changes May 20, 2021

View reviewed changes

dfreilich changed the title ~~initial create-asset-package implementation~~ Add asset package creation May 21, 2021

dwillist added 3 commits May 24, 2021 09:13

resolve merge conflicts

30d9fda

Signed-off-by: dwillist <[email protected]>

wip acceptance refactoring

ced5d7a

Signed-off-by: dwillist <[email protected]>

refactor acceptance tests

9208e55

Signed-off-by: dwillist <[email protected]>

github-actions bot added the type/chore Issue that requests non-user facing changes. label May 28, 2021

merge updates

46bba8f

Signed-off-by: dwillist <[email protected]>

dwillist force-pushed the asset_cache branch from 60fb6a0 to 46bba8f Compare June 2, 2021 13:31

dfreilich modified the milestones: 0.19.0, 0.20.0 Jun 15, 2021

dfreilich approved these changes Jun 15, 2021

View reviewed changes

Merge branch 'main' into asset_cache

d341285

jromero modified the milestones: 0.20.0, 0.21.0 Jul 14, 2021

jromero removed this from the 0.21.0 milestone Aug 25, 2021

jromero closed this Oct 6, 2021

jromero mentioned this pull request Oct 20, 2021

[RFC 0073] Add Support for Asset Cache #1055

Closed

jromero deleted the asset_cache branch April 20, 2022 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add asset package creation #1085

Add asset package creation #1085

dwillist commented Feb 19, 2021 •

edited

Loading

dfreilich commented Feb 19, 2021

codecov bot commented Feb 19, 2021 •

edited

Loading

ekcasey commented Feb 24, 2021

jromero commented Mar 11, 2021 •

edited

Loading

dwillist commented Apr 26, 2021 •

edited

Loading

dwillist commented May 20, 2021

dfreilich left a comment

dfreilich May 20, 2021 •

edited

Loading

dfreilich May 20, 2021

dfreilich May 20, 2021

dfreilich May 20, 2021

dfreilich May 20, 2021

dfreilich May 20, 2021

dfreilich May 20, 2021

dfreilich May 20, 2021

dfreilich May 20, 2021

dwillist May 27, 2021

dfreilich commented May 20, 2021

dfreilich left a comment

dfreilich commented Jun 15, 2021

jromero commented Oct 6, 2021

Add asset package creation #1085

Add asset package creation #1085

Conversation

dwillist commented Feb 19, 2021 • edited Loading

Summary

Documentation

Related

dfreilich commented Feb 19, 2021

codecov bot commented Feb 19, 2021 • edited Loading

Codecov Report

ekcasey commented Feb 24, 2021

jromero commented Mar 11, 2021 • edited Loading

dwillist commented Apr 26, 2021 • edited Loading

PR overview

Whats implemented

Whats left to do.

dwillist commented May 20, 2021

dfreilich left a comment

Choose a reason for hiding this comment

dfreilich May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfreilich commented May 20, 2021

dfreilich left a comment

Choose a reason for hiding this comment

dfreilich commented Jun 15, 2021

jromero commented Oct 6, 2021

dwillist commented Feb 19, 2021 •

edited

Loading

codecov bot commented Feb 19, 2021 •

edited

Loading

jromero commented Mar 11, 2021 •

edited

Loading

dwillist commented Apr 26, 2021 •

edited

Loading

dfreilich May 20, 2021 •

edited

Loading