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: Lifecycle Compatibility Verification #29

Closed
wants to merge 1 commit into from
Closed

RFC: Lifecycle Compatibility Verification #29

wants to merge 1 commit into from

Conversation

jromero
Copy link
Member

@jromero jromero commented Nov 4, 2019

@jromero jromero marked this pull request as ready for review November 4, 2019 23:07
@ameyer-pivotal
Copy link
Contributor

ameyer-pivotal commented Nov 5, 2019

I like how thorough this is in terms of alternatives and their pros/cons. I tend to agree that where you landed (adding a checker) is the best compromise. I thought someone floated the idea of combining all lifecycle commands into a single binary, which might reduce the complexity of adding a checker.

@ameyer-pivotal
Copy link
Contributor

I do wonder if the checker could return more than just a pass/fail. For instance, an error code for saying that the provided version is "too new" and a different error code for "too old", in case platforms want to do further logic or have it inform their own error messages (analogous to string comparisons returning +1, 0, or -1 to signify which thing was greater).

@jromero jromero changed the title Lifecycle Check Phase RFC: Lifecycle Check Phase Nov 7, 2019
@jromero
Copy link
Member Author

jromero commented Nov 12, 2019

@sclevine updated with the use of env vars instead of flag arguments. We would still need another binary to have a guaranteed entrypoint. For example, in the case that analyzer goes away or any other binary we might intend to piggy-back on.

@hone
Copy link
Member

hone commented Nov 13, 2019

From the CNB WG meeting today, do we need to take into account API checks for the launcher. Things that I'm thinking about in that contract are anything in launch.toml or how the environment gets loaded. We probably would want to fail if for some reason it isn't compatible.

@jabrown85
Copy link
Contributor

If it turns out we should be checking API versions in launcher, should we consider making an optional --platform-version flag that takes a version to compare as a pre-flight check for each binary? It could default to env of CNB_PLATFORM_API.

$ /cnb/lifecycle/detector --platform-version 0.3
> The Lifecycle's Platform API version is 0.2 which is incompatible with the request Platform API version 0.3.
$ echo $?
> 1

I know there were talks about removing a binary as a failure case. Perhaps we leave old binaries around for X amount of time in a checks-only/deprecation mode.

@jromero
Copy link
Member Author

jromero commented Nov 15, 2019

@jabrown85 can you elaborate on your findings as to what aspects of the launcher the platform interacts with?

CC: @hone

RE: launch.toml

Using the understanding that Platform API is the interface between platform and lifecycle and Buildpack API is the interface between buildpack and lifecycle. It would indicate that launch.toml is strictly Buildpack API specific. See https://github.com/buildpack/spec/blob/master/buildpack.md#launchtoml-toml

RE: launcher

The launcher has almost a completely separate interface. That which is with how the app is executed which doesn't necessarily fit in with either of the two APIs we currently have IMO.

Here is a quick glance at what the launcher cares about:

  1. CNB_PROCESS_TYPE (default: web) env var
  2. CNB_LAYERS_DIR (default: /layers) env var
  3. CNB_APP_DIR (default: /workspace) env var
  4. Loading Env Vars (detailed in Buildpack API)
  5. Loading profile.ds (detailed in Buildpack API)
  6. Determining execution command
    • process type
    • command

The platforms could leverage things like changing the default process type via CNB_PROCESS_TYPE at build time but it's not necessarily a platform specific interface.

@sclevine
Copy link
Member

I'd like to propose an additional alternative that wouldn't require adding any extra flags, commands, or binaries.

What if all the lifecycle binaries checked the environment for values in CNB_PLATFORM_API and CNB_BUILDPACK_API during their normal execution? If either of those env vars are set and the binary doesn't support the API version specified, then they could fail.

@jromero jromero changed the title RFC: Lifecycle Check Phase RFC: Lifecycle Compatibility Verification Dec 2, 2019
Signed-off-by: Simon Jones <sijones@pivotal.io>
Signed-off-by: Javier Romero <jromero@pivotal.io>
@jromero
Copy link
Member Author

jromero commented Dec 2, 2019

@sclevine I've made changes to the RFC based on your feedback.

  1. I have an open question as to why we would add this to all binaries and not just the primary (detector)? Is it due to the possibility of changing the order of the initial binary executed or something else?
  2. I don't particularly see the need for CNB_BUILDPACK_API since that's a contract between the lifecycle and the buildpacks both which live within the builder. Can you provide a use case where it would be useful?

@jromero
Copy link
Member Author

jromero commented Dec 6, 2019

Closing due to new PR #34

Long story short, I forked the repo incorrectly initially. I went ahead and fixed it but by doing so this PR became orphaned.

@jromero jromero closed this Dec 6, 2019
nebhale added a commit that referenced this pull request Mar 12, 2020
[resolves #52]

Signed-off-by: Ben Hale <bhale@pivotal.io>
zmackie pushed a commit to zmackie/rfcs that referenced this pull request Mar 30, 2020
[resolves buildpacks#52]

Signed-off-by: Ben Hale <bhale@pivotal.io>
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.

None yet

5 participants