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

Introduce explicit layer ordering RFC #322

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

schneems
Copy link

@schneems schneems commented Jan 13, 2025

@schneems schneems force-pushed the schneems/explicit-layer-ordering branch from 3ab9df2 to 890b675 Compare January 13, 2025 19:37
@buildpack-bot
Copy link
Member

Maintainers,

As you review this RFC please queue up issues to be created using the following commands:

/queue-issue <repo> "<title>" [labels]...
/unqueue-issue <uid>

Issues

(none)

@joeybrown-sf
Copy link
Contributor

Any layers with a duplicate load_order will be evaluated in alphabetic order (the same behavior as today). Any layers without a load_order will be loaded before layers with an explicit order. A load order of 0 will be loaded after layers without an explicit load_order. This default means that any explicitly ordered layers will have their PATH modifications prepended after the default layers, which has the effect of taking precedence.

I think it would be simpler to assign a load_order of 0 to layers that do not explicitly set one. That does mean that all layers with either no explicit load_order or those with an explicit load_order of 0 are treated the same and would be put into alphabetical order. But I think that makes the feature and implementation simpler. What do you think?

@schneems
Copy link
Author

schneems commented Jan 13, 2025

I think it would be simpler to assign a load_order of 0 to layers that do not explicitly set one. That does mean that all layers with either no explicit load_order or those with an explicit load_order of 0 are treated the same and would be put into alphabetical order. But I think that makes the feature and implementation simpler. What do you think?

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:
I don't have strong feelings about the defaults. Talking out loud a bit:

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 -1 and then apply the same ordering logic for layers with a conflict. My one concern with defaulting to 0 would be if people are accidentally using 0 as an explicit ordering value (starting from 0 instead of 1). So if someone was using 0 to mean "after all un-ordered-layers, but the first of the ordered" it wouldn't behave as expected. Though it also seems like 0 would be a "common sense" default and setting it to zero would be the same as saying "apply the unordered logic, but before number 1.

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

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.

3 participants