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

Move analyze phase before detect #135

Merged
merged 12 commits into from
Feb 17, 2021
Merged

Move analyze phase before detect #135

merged 12 commits into from
Feb 17, 2021

Conversation

jkutner
Copy link
Member

@jkutner jkutner commented Jan 13, 2021

Signed-off-by: Joe Kutner <jpkutner@gmail.com>
text/0000-prepare-phase.md Outdated Show resolved Hide resolved
text/0000-prepare-phase.md Outdated Show resolved Hide resolved
# What it is
[what-it-is]: #what-it-is

The prepare phase will run before all other phase, and prepare the execution environment for a buildpack build. This phase will have access to secrets and credentials used to access registries and other services.
Copy link
Member

@hone hone Jan 13, 2021

Choose a reason for hiding this comment

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

"access to secrets and credentials used to access registries and other services"

Is this something that will happen throughout the lifecycle or just in prepare like export? Should this be defined as a special level of privilege?

Copy link
Member Author

Choose a reason for hiding this comment

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

it happens in analyze, restore, and export (but not detect and build)

@hone hone requested a review from a team January 13, 2021 23:57
jkutner and others added 2 commits January 13, 2021 17:58
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Terence Lee <hone02@gmail.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
text/0000-prepare-phase.md Outdated Show resolved Hide resolved
text/0000-prepare-phase.md Outdated Show resolved Hide resolved
text/0000-prepare-phase.md Outdated Show resolved Hide resolved
text/0000-prepare-phase.md Outdated Show resolved Hide resolved
text/0000-prepare-phase.md Outdated Show resolved Hide resolved
text/0000-prepare-phase.md Outdated Show resolved Hide resolved
text/0000-prepare-phase.md Outdated Show resolved Hide resolved
text/0000-prepare-phase.md Outdated Show resolved Hide resolved
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
@jabrown85 jabrown85 changed the title Prepare Phase Move analyze phase before detect Jan 14, 2021
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
text/0000-move-analyze-phase.md Outdated Show resolved Hide resolved
text/0000-move-analyze-phase.md Show resolved Hide resolved
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
* Exit status
* Info-level logs to `stdout`
* Error-level logs to `stderr`
* Analysis metadata [`analyzed.toml`](https://github.com/buildpacks/spec/blob/main/platform.md#analyzedtoml-toml), including run-image information.
Copy link
Member

Choose a reason for hiding this comment

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

Does analyzer need to care about the cache at this stage?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think analyzer will need the cache now, actually. It is already omitted from the inputs here, but only by chance :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well hmm.. maybe analyzer does want the cache-image. But maybe not the cache-dir. So it can validate the registry keychain earlier.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is tangential to this RFC, but maybe it's worth noting that this RFC exists: https://github.com/buildpacks/rfcs/pull/21/files#diff-588fc1758ea351208954c917064a1907620d2e7a5984cd07d433c55bcd683e9b. I don't think anything in this RFC contradicts the other one, just keeping context.


The analyze phase will now run before the detect phase. The analyze phase will have access to secrets and credentials used to access registries and other services, as it does today. Analyze will no longer require a [`group.toml`](https://github.com/buildpacks/spec/blob/main/platform.md#grouptoml-toml). It will do this by splitting off some of the responsibilites into the restore phase.

## Responsibilities
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth calling out that these are new responsibilities, vs. what the analyzer does currently.

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
@hone
Copy link
Member

hone commented Jan 20, 2021

@sclevine do these updates align with what you were thinking?

We can add these in later or in another optional phase.

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
@nebhale nebhale requested a review from a team January 20, 2021 21:41
@nebhale nebhale requested a review from a team January 20, 2021 21:42
* [Stack buildpacks](https://github.com/buildpacks/rfcs/pull/111), which require a phase to read run-image mixins validation prior to detection
* Validating registry access for all images that are used can happen prior to `detector` or `builder` phases, providing faster failures for end users.

# What it is
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's worth adding in what Platform API we're going to introduce these changes.

In order to determine if cache should be restored/analyzed at all if the stack change. Future use.

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
ekcasey added a commit that referenced this pull request Feb 17, 2021
[#135]

Signed-off-by: Emily Casey <ecasey@vmware.com>
@ekcasey ekcasey merged commit dc8e65f into main Feb 17, 2021
@ekcasey ekcasey deleted the prepare-phase branch February 17, 2021 19:48
jabrown85 added a commit to buildpacks/lifecycle that referenced this pull request Apr 15, 2021
This is a re-do of #505, porting to the recently updated main branch.

[RFC](buildpacks/rfcs#135)
[Spec PR](buildpacks/spec#197)

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
jabrown85 added a commit to buildpacks/lifecycle that referenced this pull request Apr 30, 2021
Run `analyzer` before `detector`

[Related RFC](buildpacks/rfcs#135)
[Spec PR](buildpacks/spec#197)

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Co-authored-by: Natalie Arellano <narellano@vmware.com>
Co-authored-by: Yael Harel <yharel@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants