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

RFC Flatten builders #290

Merged
merged 6 commits into from
Sep 28, 2023
Merged

RFC Flatten builders #290

merged 6 commits into from
Sep 28, 2023

Conversation

dlion
Copy link
Member

@dlion dlion commented Jul 20, 2023

Signed-off-by: Domenico Luciani <[email protected]>
@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)

@hone
Copy link
Member

hone commented Jul 20, 2023

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:

  • 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).
  • Can you provide an example for exclude and include being used together?
  • I'd like to see any of the user research you all are doing being added as context into this RFC!

@tomkennedy513
Copy link

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.

text/XXX-flatten-feature.md Outdated Show resolved Hide resolved
text/XXX-flatten-feature.md Outdated Show resolved Hide resolved
@dlion
Copy link
Member Author

dlion commented Jul 25, 2023

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]
Loading

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
Loading

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 😄

@dlion dlion self-assigned this Jul 26, 2023
@hone
Copy link
Member

hone commented Jul 28, 2023

@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
Copy link
Member Author

dlion commented Jul 28, 2023

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
Copy link
Member Author

dlion commented Aug 1, 2023

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

  • Remove depth since it's not used
  • Remove --flatten --flatten-exclude since it's too complicate and it requires some mental gymnastic to be understood and used
  • Rename --flatten --flatten-include into just --flatten, it will make the feature simpler and the flatten all it's not the default mode anymore
  • It doesn't really matter if this feature is behind the experimental flag.

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

Thank you

@natalieparellano natalieparellano added team/platform scope/team RFC scoped to a sub-team as opposed to the entire project. type/rfc status/steward-assigned labels Aug 2, 2023
Signed-off-by: Domenico Luciani <[email protected]>
@dlion
Copy link
Member Author

dlion commented Aug 29, 2023

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

This section should document breaks to public API and breaks in compatibility due to this RFC's proposed changes. In addition, it should document the proposed steps that one would need to take to work through these changes. Care should be give to include all applicable personas, such as platform developers, buildpack developers, buildpack users and consumers of buildpack images.
-->
# Drawbacks
[drawbacks]: #drawbacks
Copy link
Member

Choose a reason for hiding this comment

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

It also inhibits the de-dupping of buildpack layers after they are flattened. If we distributed flattened buildpacks, the child-buildpacks can't be de-dupped. This is less of a problem for builders since we don't distribute them the same way.

Copy link
Member

@jkutner jkutner left a comment

Choose a reason for hiding this comment

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

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?)

@jjbustamante
Copy link
Member

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
Copy link
Member Author

dlion commented Aug 30, 2023

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
Copy link
Member

From our WG meeting today:

  • Let's remove from the RFC the idea of flatening a buildpack package, not because it is not possible or valuable, because it requires another conversation for the impact of doing that.
  • We want to move forward with this RFC, so once the change are done, @jkutner will take a look and if it is ok, then he can moves it to next state in the RFC process.
  • We can open another RFC for flattening a buildpack package and keep working in that without blocking flattening a builder.

@dlion dlion mentioned this pull request Sep 1, 2023
1 task
@dlion
Copy link
Member Author

dlion commented Sep 4, 2023

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: buildpacks/pack#1880 🚀


Why should we *not* do this?

It could create artifacts that are not consumable by older platforms.
Copy link
Member

Choose a reason for hiding this comment

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

This use case, only happens for platforms that create ephemeral builders (like pack and kpack) right? I was looking at how we consume builders at Heroku and I think this change would "just work".

Copy link
Member

@hone hone left a comment

Choose a reason for hiding this comment

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

Scoping this to builders minimizes the impact, which I think is a good first step. How many layers is the paketo builder at this point?

@hone
Copy link
Member

hone commented Sep 6, 2023

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?

@hone hone changed the title RFC Flatten builders and buildpacks RFC Flatten builders Sep 7, 2023
@dlion
Copy link
Member Author

dlion commented Sep 20, 2023

Hey @jkutner, is there anything else I need to do to have this PR merged? 😄

@jkutner jkutner merged commit 277c56c into buildpacks:main Sep 28, 2023
13 checks passed
@dlion dlion deleted the rfc-flatten-feature branch October 3, 2023 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope/team RFC scoped to a sub-team as opposed to the entire project. status/voting team/platform type/rfc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants