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

Merge cache and launch directories #28

Merged
merged 6 commits into from
Dec 12, 2018
Merged

Conversation

sclevine
Copy link
Member

Specifies #23.

Note: this further simplifies the initial proposal in #23 such that layers have three properties:

  1. cache
  2. build
  3. launch

Such that cached launch layers are fully cache-coherent and always restored.

Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
@sclevine sclevine changed the base branch from 24-enhanced-build-plan to master November 20, 2018 21:20
Copy link
Contributor

@nebhale nebhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of personal clarifications, but the big one is that I think the metadata can be simplified.

buildpack.md Outdated Show resolved Hide resolved
buildpack.md Show resolved Hide resolved
buildpack.md Show resolved Hide resolved
@jkutner
Copy link
Member

jkutner commented Nov 28, 2018

It's not necessarily specific to this change, but I'd prefer the bin/build signature be the following:

/bin/build <layers[EIC]> <platform[AR]> <plan[E]> 

I believe this order puts the most significant arguments first, and improves the ergonomics of the API for the majority of buildpack authors (i.e. most buildpacks won't use plan and many won't need platform, which we know from looking at existing v2 buildpacks that don't use the env dir).

There is nothing mechanically preventing a buildpack author from skipping over platform and plan, but putting them earlier forces an author to think about those arguments. It also makes introductory guides (which will likely not use platform and plan) a little hokey since they have to:

APP_DIR="$(pwd)"
LAYERS_DIR="$3"

It begs the question "what happened to $1 and $2" pretty early on when we're trying to ignore those args.

If this is accepted, bin/develop should match of course.

@nebhale
Copy link
Contributor

nebhale commented Nov 28, 2018

@jkutner I strongly disagree and think that value of the consistency in argument position (both detect and build need these arguments) is more valuable.

Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner
Copy link
Member

jkutner commented Nov 30, 2018

I've approved this as-is, but would like to see #29 incorporated.

Move layer argument to $1 for bin/build and bin/develop
@nebhale
Copy link
Contributor

nebhale commented Dec 11, 2018

Just a heads up as we do the merge into lifecycle, that the spec is still waiting on a couple of updates based on feedback.

@ekcasey
Copy link
Member

ekcasey commented Dec 11, 2018

lifecycle changes implemented in buildpacks/lifecycle#45

Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
@sclevine sclevine merged commit ea41347 into master Dec 12, 2018
@nebhale nebhale deleted the 23-the-great-layer-merge branch August 30, 2019 17:14
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.

4 participants