Remove shell-specific logic from Buildpack API #305
Conversation
buildpack.md
Outdated
| [†](README.md#linux-only)When executing a process using any execution strategy, the lifecycle SHOULD replace the lifecycle process in memory without forking it. | ||
|
|
||
| [†](README.md#linux-only)When executing a process with Bash, the lifecycle SHOULD additionally replace the Bash process in memory without forking it. | ||
| 1. The lifecycle MUST use the `execve` syscall to invoke the command with its arguments, environment, and working directory following the process outlined in the [Platform Interface Specification](platform.md). |
There was a problem hiding this comment.
The platform spec says:
MUST execute the selected process as specified in the Buildpack Interface Specfication
So I think something needs to change in the platform spec at least? But maybe we want a little more here? A potential division of details
Platform Spec describes:
-
selection of user-defined process vs buildpack-defined process
-
syntax for overriding args
Buildpack spec:
- Says that in the buildpack-defined process case: command+args will be executed but that args may be overridden with user provided args at the platform's discretion (basically what you have in the data format section below but moved up here so it's easier to find?)
There was a problem hiding this comment.
Would it be worth having a section for the process definition interface? Similar to what we have for exec.d?
There was a problem hiding this comment.
Good catch. tl;dr: I agree with the above, and I think we should:
- consolidate the execution details in platform spec, without pointer back to buildpack spec.
- add a table here with the command/args distinction.
- note here (maybe a footnote in the table) that it's at the platform's discretion.
Some more color:
I don't think the platform spec should point back to the buildpack spec about execution. The buildpack can express intention, but it is really up to the platform spec how to execute. Perhaps that is the main message that should go here...this is what these things mean, but it is up to the platform to execute.
For example, in implementing this RFC, when we change command from a string to a list of strings, platforms with API < 0.10 won't know that some args are override-able (args) and some aren't (command[1:]), as there is no way to express this distinction in the current metadata.toml format. (I walked through this in almost absurd detail in this miro board: https://miro.com/app/board/uXjVO8dPGUU=/?share_link_id=509704965141 )
There was a problem hiding this comment.
Took a shot at these items if you'd like to give it another pass.
There was a problem hiding this comment.
Love the table @mboldt 🎉 the absurd level of detail would be great for our docs! Maybe in the migration guide?
|
sorry, not sure why github doubled all my comments :/ resolving the dupes... |
There was a problem hiding this comment.
This generally LGTM. Would like to see more details around the execution details as @ekcasey mentioned.
I think we should also make it clear from a BP Author's perspective on
what happens when they specify things in the command array v/s args array and how it interpolates with the user provided args (either via docker or k8s)
buildpack.md
Outdated
| - The first element of `command` is a path to an executable or the file name of an executable in `$PATH`. | ||
| - Any remaining elements of `command` are arguments that are always passed directly to the executable. | ||
| - MAY specify an `args` list to be passed directly to the specified executable, after arguments specified in `command`. | ||
| - The `args` list is a default list of arguments that may be overridden by the user. |
There was a problem hiding this comment.
Can there be a pointer here back to the platform spec? I think that would address @samj1912's comment here: #305 (review)
There was a problem hiding this comment.
Something like this? b9e6e34
|
@mboldt nice work! I left a couple comments but in general this LGTM |
buildpack.md
Outdated
| | Field | Type | Definition | | ||
| |---------------|-----------------|----------------------------------------------------------------------------------------------| | ||
| | `type` | String | An identifier for the process | | ||
| | `command` | Array of string | The command to execute, followed by arguments that should always be provided [^command-args] | | ||
| | `args` | Array of string | Default arguments to the command that can be overridden by the user | | ||
| | `default` | Boolean | If `true`, use this as the default process for the app image | | ||
| | `working-dir` | Path | Working directory for this process in the app image | |
There was a problem hiding this comment.
This information is somewhat duplicated in the launch.toml section below. I wonder if we could consolidate it in one place.
There was a problem hiding this comment.
Consolidated in launch.toml section in de2636c
Closes #244 Signed-off-by: Mikey Boldt <[email protected]>
Signed-off-by: Mikey Boldt <[email protected]>
Signed-off-by: Mikey Boldt <[email protected]>
Co-authored-by: Natalie Arellano <[email protected]> Signed-off-by: Mikey Boldt <[email protected]>
Co-authored-by: Natalie Arellano <[email protected]> Signed-off-by: Mikey Boldt <[email protected]>
Co-authored-by: Natalie Arellano <[email protected]> Signed-off-by: Mikey Boldt <[email protected]>
Signed-off-by: Mikey Boldt <[email protected]>
Signed-off-by: Mikey Boldt <[email protected]>
Signed-off-by: Mikey Boldt <[email protected]>
Co-authored-by: Natalie Arellano <[email protected]> Signed-off-by: Mikey Boldt <[email protected]>
|
I believe this PR has all the required approvals. Can we merge it in? |
Per RFC 0093.
Closes #244
Signed-off-by: Mikey Boldt [email protected]