-
Notifications
You must be signed in to change notification settings - Fork 71
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
Introduce explicit layer ordering RFC #322
base: main
Are you sure you want to change the base?
Introduce explicit layer ordering RFC #322
Conversation
Signed-off-by: Schneems <[email protected]>
3ab9df2
to
890b675
Compare
Maintainers, As you review this RFC please queue up issues to be created using the following commands:
Issues(none) |
I think it would be simpler to assign a |
Short: Sounds good. Making it zero would work if it would help out implementation. I don't have strong reservations or need to differentiate between the two values. Updated. Long: As I described it, if the spec says that it cannot be a negative number in the TOML then you could implement it by defaulting to If I was implementing to the spec in Rust and it says "this value must be a positive integer" then would like that I could model that in a struct directly instead of having one representation for "this is compliant with the spec" and then having to transform it to a signed integer for the comparison. I see that benefit. I'm also realizing that maybe what you're hinting at is that instead of doing it in code, the lifecycle could insert it in the TOML files when it's missing after buildpack execution and then all the tooling could have strong guarantees it would always exist. That makes sense and seems valuable to be consistent. I'm for it |
Signed-off-by: Schneems <[email protected]>
78de4eb
to
04c8e71
Compare
7a3bdaf
to
c8881c4
Compare
…r some binaries Signed-off-by: Schneems <[email protected]>
c8881c4
to
3dbe584
Compare
We talked about this in the meeting. I'm not 100% certain I'm capturing the original suggestion and might update this with more details, but this is the general idea. I recall another benefit was mentioned for this scheme but didn't capture it and it's slipped my memory. Signed-off-by: Schneems <[email protected]>
47b272c
to
df2500c
Compare
Updated after discussion from the working group meeting. |
The voting has begun and will end on Feb 13th. |
text/0133-explicit-layer-order.md
Outdated
``` | ||
build = true | ||
launch = true | ||
load_order = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After peeking at the code to refresh my memory - the launch.toml
for each layer doesn't exist in the final run image.
launcher
reads the buildpacks in the order they are written in metadata.toml
when processing. It then reads the directories in alphanumeric order within each buildpack directory.
lifecycle
would need to persist this information elsewhere, like config/metadata.toml
so that launcher
could iterate the directories in the specified order.
[[buildpacks]]
id = "<buildpack ID>"
version = "<buildpack version>"
api = "<buildpack API version>"
optional = false
ordered-layers = ["<ruby-layer-1>", "<ruby-layer-3>"]
Co-authored-by: Jesse Brown <[email protected]> Signed-off-by: Richard Schneeman <[email protected]>
While TOML supports integers, the current implementation details of the CNB project mean that it is converted to JSON and then back to TOML which loses type information https://github.com/buildpacks/lifecycle/issues/884. This issue may make comparing values for equivalence harder. This behavior prevents me from strongly suggesting that the `load_order` key should be a TOML Integer type. | ||
|
||
# Alternatives | ||
[alternatives]: #alternatives |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see an alternative listed
Alternative: Order file in each layer
The individual buildpack author can emit <layer-dir>/CNB_LAUNCH_LAYER_ORDER_INDEX
with a numeric value. From an implementation perspective, I think I like it better than launch.toml
or a aggregate file at the buildpack directory. Being a file on disk means it doesn't have to be handled in intermediate files or show up in OCI image labels.
I don't know what implications it has on a buildpack author and if there would be any concerns around re-ordering layers and cache busting. I'd love for you weigh in @schneems
Rendered