Remove shell-specific logic and update related command/arg handling #306
Conversation
| # OR | ||
| /cnb/lifecycle/launcher [--] [<cmd> <arg>...] | ||
| /cnb/lifecycle/launcher [<cmd> <args>...] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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=/
| - **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>`. |
There was a problem hiding this comment.
| - **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>`. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
@mboldt left a couple of suggestions. I think we'd want to update #306 (comment) so that it's accurate, but otherwise this looks pretty good to me. |
Closes #245 Signed-off-by: Mikey Boldt <[email protected]>
Signed-off-by: Mikey Boldt <[email protected]>
Signed-off-by: Mikey Boldt <[email protected]>
We still need it for older Buildpack API versions. Signed-off-by: Mikey Boldt <[email protected]>
Signed-off-by: Mikey Boldt <[email protected]>
Modeled after Kubernetes. See https://kubernetes.io/docs/tasks/inject-data-application/define-interdependent-environment-variables/ Signed-off-by: Mikey Boldt <[email protected]>
They get combined into command in the new metadata.toml format. 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]>
Co-authored-by: Natalie Arellano <[email protected]> Signed-off-by: Mikey Boldt <[email protected]>
|
I have approved the related Buildpack API changes. But as discussed in WG the launcher changes have user facing impact and we probably want to hold off on this for now? |
Per RFC 0093.
Closes #245
Signed-off-by: Mikey Boldt [email protected]