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.

Sign up for GitHub

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

Jump to bottom

Remove shell-specific logic and update related command/arg handling #306

Closed
wants to merge 14 commits into from
Closed

Remove shell-specific logic and update related command/arg handling #306

Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c7b1541
Remove shell-specific logic and update related command/arg handling
mboldt Apr 15, 2022
5a852a2
Reintroduce `direct` to maintain support for older buildpacks
mboldt May 21, 2022
feff99d
Add .profile deprecation
mboldt May 21, 2022
ef1a830
Restore `direct` in metadata.toml
mboldt May 24, 2022
0de4920
Add pointer to .profile utility buildpack
mboldt May 31, 2022
4f24b75
Describe launcher environment variable interpolation
mboldt Jun 1, 2022
b5df27c
Describe conversion of command and args from older buildpacks
mboldt Jun 1, 2022
f7681d7
Fix typo
mboldt Jun 1, 2022
072bb91
Make style more consistent
mboldt Jun 2, 2022
585ebf0
Update platform.md
mboldt Jun 9, 2022
4be60b3
Update platform.md
mboldt Jun 9, 2022
14eb3bb
Update platform.md
mboldt Jun 9, 2022
f3b13d8
Add indent level to clarify the intended flow
mboldt Jun 10, 2022
dbb4321
Update platform.md
mboldt Jun 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 45 additions & 22 deletions platform.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ Examples of a platform might include:

## Platform API Version

This document specifies Platform API version `0.9`.
This document specifies Platform API version `0.10`.

Platform API versions:
- MUST be in form `<major>.<minor>` or `<major>`, where `<major>` is equivalent to `<major>.0`
Expand Down Expand Up @@ -499,6 +499,7 @@ Usage:
- The lifecycle SHALL execute all buildpacks in the order defined in `<group>` according to the process outlined in the [Buildpack Interface Specification](buildpack.md).
- The lifecycle SHALL add all invoked buildpacks to`<layers>/config/metadata.toml`.
- The lifecycle SHALL aggregate all `processes` and `slices` returned by buildpacks in `<layers>/config/metadata.toml`.
- For buildpacks implementing a Buildpack API where the `direct` field on process types is supported, the lifecycle SHALL combine each process' `command` and `args` in `launch.toml` into `command` in `<layers>/config/metadata.toml`, and leave `args` empty in `<layers>/config/metadata.toml`.
- The lifecycle SHALL record the buildpack-provided default process type in `<layers>/config/metadata.toml`.
- The lifecycle SHALL treat `web` processes defined by buildpacks implementing Buildpack API < 0.6 as `default = true`.

Expand Down Expand Up @@ -735,43 +736,52 @@ Usage:
#### `launcher`
Usage:
```
/cnb/process/<process-type> [<arg>...]
/cnb/process/<process-type> [<args>...]
# OR
/cnb/lifecycle/launcher [--] [<cmd> <arg>...]
/cnb/lifecycle/launcher [<cmd> <args>...]
Copy link
Member

Choose a reason for hiding this comment

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

This is a very significant end user change. We should be careful around this since users won't be able to control this. How do you imagine this working in general since processes are global and not buildpack specific? Would processes contributed by the older buildpack apis still allow for the old launcher approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I do expect process types contributed by older buildpack APIs to maintain the old launcher approach (that is, allowing direct=false to spawn using a shell), with the caveat that the launcher will not source .profile scripts (though we will develop the .profile buildpack and systems buildpack feature to avoid platform regressions for that). I did not have this in my first commit here, but should be there now.

I will review/refresh my thinking on the buildpack -> launch.toml -> metadata.toml -> launcher to make sure it lines up without the connection from the global process in metadata.toml back to the buildpack API version. I think this is part of the concern you're expressing. Let me know if I misread.

(Note, I think you're speaking more generally here as opposed to just the couple lines that this comment got associated with...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewing, the platform can link the launch.toml to its buildpack, and set direct in metadata.toml appropriately. I wrote out an almost-ridiculous number of permutations to get my head straight around this, if you want to take a look: https://miro.com/app/board/uXjVO8dPGUU=/

```
##### Inputs
| Input | Environment Variable | Default Value | Description |
|------------------------------------|----------------------|---------------|-----------------------------------------------------------|
| `<app>` | `CNB_APP_DIR` | `/workspace` | Path to application directory |
| `<layers>` | `CNB_LAYERS_DIR` | `/layers` | Path to layer directory |
| `<process-type>` | | | `type` of process to launch |
| `<direct>` | | | Process execution strategy |
| `<cmd>` | | | Command to execute |
| `<args>` | | | Arguments to command |
| `<layers>/config/metadata.toml` | | | Build metadata (see [`metadata.toml`](#metadatatoml-toml) |
| `<layers>/<buildpack-id>/<layer>/` | | | Launch Layers |

A command (`<cmd>`), arguments to that command (`<args>`), a working directory (`<working-dir>`), and an execution strategy (`<direct>`) comprise a process definition. Processes MAY be buildpack-defined or user-defined.
A command (`<cmd>`), arguments to that command (`<args>`), and a working directory (`<working-dir>`) comprise a process definition. Processes MAY be buildpack-defined or user-defined.
natalieparellano marked this conversation as resolved.
Show resolved Hide resolved

The launcher:
- MUST derive the values of `<cmd>`, `<args>`, `<working-dir>`, and `<direct>` as follows:
natalieparellano marked this conversation as resolved.
Show resolved Hide resolved
- **If** the final path element in `$0`, matches the type of any buildpack-provided process type
- `<process-type>` SHALL be the final path element in `$0`
- The lifecycle:
- MUST select the process with type equal to `<process-type>` from `<layers>/config/metadata.toml`
- MUST set `<working-dir>` to the value defined for the process in `<layers>/config/metadata.toml`, or to `<app>` if not defined
- MUST append any user-provided `<args>` to process arguments
- **Else**
- **If** `$1` is `--`
- **If** the final path element in `$0`, matches the type of any buildpack-provided process type
- `<process-type>` SHALL be the final path element in `$0`
- **If** the buildpack API version supports the `direct` field on process definitions, **and** the `direct` field for this process definition is set to `false`
- `<direct>` is `false`
- **Else**
- `<direct>` is `true`.
- The lifecycle:
- MUST select the process with type equal to `<process-type>` from `<layers>/config/metadata.toml`
- MUST set `<cmd>` to the first element of `command` in the selected process type.
- MUST set `<args>` to any remaining elements of `command` in the selected process type, followed by:
- **If** there are user-provided `<args>`, the user-provided `<args>`
- **Else** any `args` defined in the selected process type
- MUST set `<working-dir>` to the `working-dir` defined for the selected process type, or to `<app>` if not defined
- **Else**
- `<direct>` SHALL be `true`
- `<cmd>` SHALL be `$2`
- `<args>` SHALL be `${@3:}`
- `<cmd>` SHALL be the user-provided `<cmd>`
- `<args>` SHALL be the user-provided `<args>`
- `<working-dir>` SHALL be `<app>`
- **Else**
- `<direct>` SHALL be `false`
- `<cmd>` SHALL be `$1`
- `<args>` SHALL be `${@2:}`
- `<working-dir` SHALL be `<app>`

- **If** `<direct>` is `true`
- MUST invoke the command `<cmd>` with its arguments `<args>`, environment `<env>`, and working directory `<working-dir>`.
- **Else** `<direct>` is `false`, and
- MUST invoke the command `<cmd>` using a shell with its arguments `<args>`, environment `<env>`, and working directory `<working-dir>`.
Comment on lines +777 to +780
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- **If** `<direct>` is `true`
- MUST invoke the command `<cmd>` with its arguments `<args>`, environment `<env>`, and working directory `<working-dir>`.
- **Else** `<direct>` is `false`, and
- MUST invoke the command `<cmd>` using a shell with its arguments `<args>`, environment `<env>`, and working directory `<working-dir>`.
- MUST invoke the command `<cmd>` with its arguments `<args>`, environment `<env>`, and working directory `<working-dir>`.

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 right... right? You can't have a direct process if it wasn't defined by a buildpack on an older buildpack api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It think we need to consider both here...I updated the indentation in 493bb4a to try to show the intention.

Basically, above this part we define how to set direct, and here we describe what needs to happen based on the value of direct. Does that make sense?


- MUST replace all occurrences of `$(<env>)` in `<args>`, where `<env>` is the name of a variable defined in the launch environment, with the value of the environment variable after applying buildpack-provided environment modifications, before it launches the process.
If the environment variable `<env>` is undefined, MUST treat `$(<env>)` like a normal string and perform no replacements.
- MUST NOT replace any environment variable references escaped with a double `$`, i.e., `$$(<env>)`.

##### Outputs
If the launcher errors before executing the process it will have one of the following error codes:
Expand Down Expand Up @@ -933,7 +943,7 @@ optional = false

[[processes]]
type = "<process type>"
command = "<command>"
command = ["<command>"]
natalieparellano marked this conversation as resolved.
Show resolved Hide resolved
args = ["<arguments>"]
direct = false
working-dir = "<working directory>"
Expand Down Expand Up @@ -1042,7 +1052,9 @@ Where:
"processes": [
{
"type": "<process-type>",
"command": "<command>",
"command": [
"<command>"
],
"args": [
"<args>"
],
Expand Down Expand Up @@ -1153,3 +1165,14 @@ Where:
}
```
This label MUST contain the JSON representation of [`project-metadata.toml`](#project-metadatatoml-toml)

## Deprecations

### Sourcing `.profile` or `.profile.bat` Scripts

_Unsupported in Platform API 0.10._

The launcher no longer sources `<app>/.profile` or `<app>/.profile.bat` files if present.
The launcher previously sourced these files when the command was run in a shell (i.e., `direct = false`), before executing the command in the same shell.
mboldt marked this conversation as resolved.
Show resolved Hide resolved

Platform operators who wish to continue supporting `.profile` or `.profile.bat` scripts to avoid regressions can use the [.profile utility buildpack](https://github.com/buildpacks/profile).