buildpacks / rfcs

RFCs for Cloud Native Buildpacks
Apache License 2.0
56 stars 71 forks source link

RFC Flatten builders #290

Closed dlion closed 1 year ago

dlion commented 1 year ago

Readable

buildpack-bot commented 1 year ago

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)

hone commented 1 year ago

Thanks for writing this up! For a small nit can you rename your file to text/XXX-flatten-feature.md.

As mentioned in WG today:

tomkennedy513 commented 1 year ago

In the case of flattening a builder, how would you determine which layers to flatten? If it is just by buildpack include/exclude flags, what would a user expect to happen when a buildpack is shared across multiple meta buildpacks. Also wondering how the flattening of builders would come into play in a platform like kpack since it doesn't use pack to build builders.

dlion commented 1 year ago

Thanks for writing this up! For a small nit can you rename your file to text/XXX-flatten-feature.md

The file is already called https://github.com/dlion/rfcs/blob/rfc-flatten-feature/text/XXX-flatten-feature.md did I miss anything? πŸ€”

Is there a need for flattening buildpacks or just builders? My concern about flattening buildpacks is if it's distributed this way, then it removes the flexibility for the builder author (if they don't have the layer limit issue for their composition).

Uhm, I don't know about the specific need around one or another choice since I wasn't there (I remember that @jjbustamante told me that he has been asked for this implementation) , maybe @natalieparellano has some context about it.

Can you provide an example for exclude and include being used together?

Uhm, let’s say we have something like: BP 1, BP 2, BP 3, BP 4, BP 5, BP 6

flowchart TD
    A[CB1]
    A --> BP1[BP1]
    A --> BP2[BP2]
    A  --> BP3[BP3]
    A --> BP4[BP4]
    A --> BP5[BP5]
    A --> BP6[BP6]

and I want just to specifically merge BP 1 and BP2, but exclude BP 5 and leave the rest flatten to obtain something like: BP1-BP2 BP5 BP3-BP4-BP6

flowchart TD
A[CB1]
A --> BP1BP2[BP1-BP2]
A --> BP3BP4[BP3-BP4-BP6]
A --> BP5

Does it make sense?

I'd like to see any of the user research you all are doing being added as context into this RFC! We are planning to have the session this Thursday, I'll keep it updated @hone πŸ˜„

hone commented 1 year ago

@dlion

The file is already called https://github.com/dlion/rfcs/blob/rfc-flatten-feature/text/XXX-flatten-feature.md did I miss anything? thinking

Sorry, i meant it should be rename from that with 0000 instead of XXX as talked about in the README.

dlion commented 1 year ago

Sorry, i meant it should be rename from that with 0000 instead of XXX as talked about in the README.

Ops my bad, I misinterpreted the sentence in the readme where 'my-feature' is descriptive. don't assign an RFC number yet as Don't even put 000. I'll fix it asap, thanks @hone

dlion commented 1 year ago

I've just updated this RFC with the feedback we've got from some of our stakeholders:

Here a partial image of the MIRO board we used to ran the exercise with our users: image

Thank you

dlion commented 1 year ago

Hi @hone, is this good to be voted? I'd like to start the voting window, let's say for a period of 1 week, does it sound reasonable? /cc @jjbustamante @natalieparellano

jjbustamante commented 1 year ago

Could we limit this RFC to pack builder create with flattened layers?

It seems like that's really the long pole, since that's where most people would accumulate more than 127 layers (are there really examples of individual buildpacks that have more 127 child buildpacks?)

I think it makes sense, just want to confirm @ForestEckhardt Paketo is flattening only the builders, right?

As an example, I took the Java buildpack from Paketo and count the number of layers there:

> crane manifest gcr.io/paketo-buildpacks/java | jq '.layers | length'
27

I got 27, so I think ti is fine.

dlion commented 1 year ago

Could we limit this RFC to pack builder create with flattened layers?

I think it's a great idea, as I said during our sync would be better to just focus on the builder part to be sure we focus on deliverying value to our users. From that we can start collecting feedback and if this functionality it is needed for buildpacks as well we can considerate it right away. πŸš€

I'll modify the RFC accordingly and start creating the first issue about it. Thanks for the feedback everyone!

jjbustamante commented 1 year ago

From our WG meeting today:

dlion commented 1 year ago

Hi all, I updated the RFC removing any reference that might not be related to a Builder. Let me know if it is fine πŸ˜„ In the meantime I also created the issue related to this RFC: https://github.com/buildpacks/pack/issues/1880 πŸš€

hone commented 1 year ago

I'm moving this to voting with a close of Sept 13th. @jkutner I'll be out for half of next week, so can you handle finalizing this?

dlion commented 1 year ago

Hey @jkutner, is there anything else I need to do to have this PR merged? πŸ˜„