-
Notifications
You must be signed in to change notification settings - Fork 71
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
Launcher Arguments #84
Conversation
This RFC proposes a change to the lifecycle execution behavior to allow arguments to be appended to buildpack-contributed process types. Signed-off-by: Ben Hale <[email protected]>
I really like this idea overall, but I think the big outstanding question here is the UX for As a simple/contrived example, imagine "hello world" type process. A directly executed version of this command would look like the following:
If we implemented this RFC and I passed "world" to the launcher the command essentially becomes
what gets executed by the launcher is essentially just BUT a
Passing the same "world" argument to the launcher would result in
and the launcher would essentially execute
AND even the above doesn't work in practice when read from a
(This last part is probably to some quoting, toml parsing, nested bash complexity in the launcher implementation, that I don't fully understand, and might independently resolvable. If we fix quoting edge cases this UX might be okay? Folks that want to construct |
@ekcasey I'd go with something significantly simpler, appending the arguments, space delimited, to the command itself. |
@nebhale appending space delimited arguments to the command was an idea I was interested in for |
I'm 👍 on this idea as well, but pending resolving the |
I'm strongly opposed to modifying the command automatically. It seems very confusing to provide discreet arguments that don't map to proper argument inputs. Could we just disallow My other concern is that overloading the first argument could lead to unexpected behavior. E.g., if an argument happens to match something on the path, the command would unexpected execute that binary instead. Could we use environment variables to provide process-type selection and/or shell command overrides instead of trying to fit all of this into a single list? |
@ekcasey in the example you provide:
Do you mean to say |
Request for prior art: what other tools solve this problem? |
https://docs.npmjs.com/cli/run-script#description allows arguments via |
+1 to this idea. I've been wanting to be able to define |
* require process type or override to be set via env var Signed-off-by: Emily Casey <[email protected]>
Signed-off-by: Ben Hale <[email protected]>
Signed-off-by: Emily Casey <[email protected]>
Signed-off-by: Emily Casey <[email protected]>
Signed-off-by: Emily Casey <[email protected]>
# Alternatives | ||
[alternatives]: #alternatives | ||
|
||
Previously in Cloud Foundry which did not support a system like this, the solution that was chosen was to use an environment variable to represent arguments: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant did here.
Previously in Cloud Foundry which did not support a system like this, the solution that was chosen was to use an environment variable to represent arguments: | |
Previously in Cloud Foundry which did support a system like this, the solution that was chosen was to use an environment variable to represent arguments: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, "a system like this", refers to one that can accept additional args. CF did not support that. You were forced to use environment variables to change the process.
### Exporter | ||
When `exporter` creates an app image it will create one symlink per buildpack-contributed process type. The symlink files will be named `/cnb/process/<process-type>` and will point at the launcher. | ||
|
||
The `exporter` will prepend `/cnb/process` to the image `PATH`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we discussed that this PATH
change would be restricted to starting the image and stripped from the env
of the actual process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, I'll update that
`docker run --entrypoint /cnb/process/web <image>` | ||
Will both launch the process with `type` `web` | ||
|
||
Buildpack-contributed processes will be forbidden from having `type` equal to `"launcher"`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we saying that buildpack authors will have no way of getting the --entrypoint
to be launcher
if any other buildpack defined a process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to allow ambiguity in $0
between a process with type launcher
and the normal launcher
* Adds requirement that launcher removes /cnb/process and /cnb/lifecycle from the PATH before process execution Signed-off-by: Emily Casey <[email protected]>
Final Comment Period with merge disposition, closing on 26 June, 2020. |
Simple question, but to make sure I understand:
rather than what we currently have to do ( |
Assuming the built image has a |
[#84] Signed-off-by: Ben Hale <[email protected]>
Signed-off-by: Javier Romero <[email protected]>
Signed-off-by: Javier Romero <[email protected]>
Signed-off-by: Javier Romero <[email protected]>
Signed-off-by: Javier Romero <[email protected]>
Signed-off-by: Javier Romero <[email protected]>
Signed-off-by: Javier Romero <[email protected]>
Signed-off-by: Javier Romero <[email protected]>
1. A GitHub Action that monitors PRs for the purpose of easier issue management: 1. Creates an initial comment instructing maintainers that is updated as issues are queued. 2. Parses comments for `/queue-issue` or `/unqueue-issue` to track issues to create. 2. A `node` based CLI to query and create queued issues (currently necessary to integrate with `merge-rfc.sh`) 3. Changes to `merge-rfc.sh` to query and create queued issues Signed-off-by: Javier Romero <[email protected]>
Add Issues Generation as per RFC #84
1. A GitHub Action that monitors PRs for the purpose of easier issue management: 1. Creates an initial comment instructing maintainers that is updated as issues are queued. 2. Parses comments for `/queue-issue` or `/unqueue-issue` to track issues to create. 2. A `node` based CLI to query and create queued issues (currently necessary to integrate with `merge-rfc.sh`) 3. Changes to `merge-rfc.sh` to query and create queued issues Signed-off-by: Javier Romero <[email protected]> Signed-off-by: Halil İbrahim ceylan <[email protected]>
This RFC proposes a change to the lifecycle execution behavior to allow arguments to be appended to buildpack-contributed process types.
Rendered