-
Notifications
You must be signed in to change notification settings - Fork 0
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
make ready "Prepare Phase RFC" #1
Conversation
Signed-off-by: Halil <[email protected]>
# Definitions | ||
[definitions]: #definitions | ||
|
||
* __stack descriptor__ - an enhanced version of the [`stack.toml`](https://github.com/buildpacks/spec/blob/main/platform.md#stacktoml-toml) |
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.
We should describe how it's enhanced. (I'm not sure myself...)
The prepare phase would support the following features and capabilities: | ||
* [Stack buildpacks](https://github.com/buildpacks/rfcs/pull/111), which require a phase to read run-image mixins validation prior to detection | ||
* [Inline buildpacks](https://github.com/buildpacks/rfcs/blob/main/text/0048-inline-buildpack.md), which require parsing of the `project.toml` in the lifecycle | ||
* [Lifecycle configuration](https://github.com/buildpacks/rfcs/pull/128) |
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.
It might be good to list exactly what would be configured. Though perhaps it's worth noting, the linked RFC was closed (I forget exactly why). Maybe it isn't a concern anymore.
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.
Actually i am little bit confusing about what would be configured. This is Draft RFC is little bit old and a lot of links are already merged or closed. The issue is not providing any technical information (except your comment). What should i do to find correct list for here ?
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.
Let's take it out for now, if there's a need surely others will suggest it :)
* Stack validation, to ensure that a new run-image is campatible with the previous app image | ||
* Retrive run-image mixins, which will be use dby subsequent phases | ||
* Validation of registry credentials, to avoid a long build that fails during export phase | ||
* Parsing the project descriptor and performance various operations based on its contents, include: |
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.
Would it be possible to list all of the features? It might be good to break them out by:
- whether they are currently implemented in pack (yes/no)
- whether they are "platform" things vs. "buildpack" things (things a buildpack could do - e.g., modifying environment variables)
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.
Here is the project descriptor spec: https://github.com/buildpacks/spec/blob/main/extensions/project-descriptor.md
And here is where pack parses the project descriptor: https://github.com/buildpacks/pack/blob/f6b78274f8f6aa5c659db509a14230f1d1610abc/internal/commands/build.go#L66
If you're having any questions about the part, the folks in #pack-cli will be able to help :)
* Run-image | ||
* Stack ID |
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.
These might not make sense if we are removing some responsibilities from the prepare phase (see comments re: analyze reading run-image mixins, etc.)
* Stack descriptor | ||
* Analysis metadata |
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.
Same comment as https://github.com/haliliceylan/rfcs/pull/1/files#r646834293
|
||
The prepare phase would be implemented as the `/cnb/lifecycle/preparer` binary. A platform MUST execute this phases either by invoking the `/cnb/lifecycle/preparer` binary or by executing `/cnb/lifecycle/creator`. | ||
|
||
The `preparer` binary will have access to the [`Keychain`](https://github.com/buildpacks/lifecycle/blob/main/auth/env_keychain.go), and MUST NOT execute arbitrary code provided by either the buildpack user or buildpack author. |
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.
(deleted)
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.
Maybe it's also worth noting if the preparer will run as privileged or not (I don't think it needs to).
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.
Removed my suggestion, I forgot that we need registry auth to download buildpacks.
# Alternatives | ||
[alternatives]: #alternatives | ||
|
||
- [Reverse the order of analyze and detect phases](https://github.com/buildpacks/spec/pull/172) |
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.
This is the one we went with to accomplish a subset of what's listed :)
* Yet another lifecycle phase | ||
|
||
# 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 think both of the listed alternatives could be removed, because they are dealing with the problem of "who is reading the run-image mixins".
# Prior Art | ||
[prior-art]: #prior-art | ||
|
||
- [Tekton prepare step](https://github.com/tektoncd/catalog/blob/11a17cfe87779099b0b61be3f1e496dfa79646b3/task/buildpacks-phases/0.1/buildpacks-phases.yaml#L61-L78) |
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.
It might be worth noting that this "prepare" step doesn't really do any of the things we want the preparer to do. I think it's just included here because it's called prepare :)
# Unresolved Questions | ||
[unresolved-questions]: #unresolved-questions | ||
|
||
- Do we still need `analyzer`, or can the remaining parts of analyze phase be rolled into restore phase? |
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.
This one could be removed I think.
# Spec. Changes | ||
[spec-changes]: #spec-changes | ||
|
||
See [buildpacks/spec PR #176](https://github.com/buildpacks/spec/pull/176) |
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.
Outdated now, we could just remove it. But maybe it's worth noting that this will require a platform API bump, with no impact to the buildpack API.
Co-authored-by: Natalie Arellano <[email protected]>
Co-authored-by: Natalie Arellano <[email protected]>
Co-authored-by: Natalie Arellano <[email protected]>
No description provided.