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

Restore cached launch layers not found in appLayers #1346

Conversation

pbusko
Copy link
Contributor

@pbusko pbusko commented Apr 23, 2024

Summary

In case if a layer is found in cache and marked as launch = true and cache = true, it will be restored if not found in the appLayers.

This case is possible if -previous-image is not set/found, however a -cache-dir is provided (backed by persistent volumes etc.)

Release notes


Related

Resolves #___


Context

@pbusko pbusko requested a review from a team as a code owner April 23, 2024 12:29
@pbusko pbusko force-pushed the restore-cached-launch-layers-not-in-app branch 2 times, most recently from 1ae24b1 to c29c50b Compare April 23, 2024 14:53
@loewenstein
Copy link

@natalieparellano @jabrown85 Did you have a chance to take a look?

@natalieparellano
Copy link
Member

natalieparellano commented Apr 30, 2024

@loewenstein taking a look...

I hate to be that person, but I think this may require a spec change, as the spec is actually quite specific about where the layer metadata comes from: https://github.com/buildpacks/spec/blob/main/buildpack.md#layer-types

That said, having been around when this table was added, I believe its value is simply to clarify how the lifecycle works, as even CNB contributors could not keep track of this behavior. I don't think buildpack authors (or platform authors for that matter) care or need to care where this metadata comes from (maybe we could just update the table to remove that additional detail).

@natalieparellano
Copy link
Member

@pbusko I think if you rebase onto the latest main that will fix the Codecov issue

@pbusko
Copy link
Contributor Author

pbusko commented May 2, 2024

@natalieparellano unfortunately after rebase the unit tests started to fail on windows platform (is the issue is more general, since #1347 experiences it as well?)

The PR to the spec: buildpacks/spec#406

@natalieparellano
Copy link
Member

@pbusko that looks like a flake. We can re-kick it

I hate to be that person, but I think this may require a spec change, as the spec is actually quite specific about where the layer metadata comes from: https://github.com/buildpacks/spec/blob/main/buildpack.md#layer-types

That said, having been around when this table was added, I believe its value is simply to clarify how the lifecycle works, as even CNB contributors could not keep track of this behavior. I don't think buildpack authors (or platform authors for that matter) care or need to care where this metadata comes from (maybe we could just update the table to remove that additional detail).

We talked about this in Working Group today. We agreed that (1) the current level of detail can be removed from the spec, and (2) it's still a behavior change for our platform users and would be better if gated behind a Platform API version bump.

Would you be amenable to this? ^^

Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>
Signed-off-by: Pavel Busko <pavel.busko@sap.com>
@pbusko pbusko force-pushed the restore-cached-launch-layers-not-in-app branch 2 times, most recently from b2db07b to 0c92e65 Compare May 3, 2024 13:22
Signed-off-by: Pavel Busko <pavel.busko@sap.com>
@pbusko pbusko force-pushed the restore-cached-launch-layers-not-in-app branch from 0c92e65 to a21dc68 Compare May 5, 2024 05:51
@natalieparellano natalieparellano added this to the lifecycle 0.20.0 milestone May 6, 2024
@natalieparellano
Copy link
Member

This should go in Platform 0.14: https://github.com/buildpacks/spec/milestones/Platform%200.14

(the current issues in that milestone were added long ago and could be kicked out to 0.15 if we want to move this sooner)

@pbusko
Copy link
Contributor Author

pbusko commented Jun 20, 2024

@natalieparellano is there anything blocking this PR and buildpacks/spec#406 from being merged?

@natalieparellano
Copy link
Member

@pbusko thanks for the ping - I'll review this today.

@loewenstein-sap
Copy link

Needs a second approval? @natalieparellano @jabrown85 ^^

@natalieparellano natalieparellano merged commit f2a3bd7 into buildpacks:main Jul 3, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants