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

Launcher Arguments #84

Merged
merged 8 commits into from
Jun 26, 2020
Merged

Launcher Arguments #84

merged 8 commits into from
Jun 26, 2020

Conversation

nebhale
Copy link
Contributor

@nebhale nebhale commented Jun 2, 2020

This RFC proposes a change to the lifecycle execution behavior to allow arguments to be appended to buildpack-contributed process types.

Rendered

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]>
@nebhale nebhale requested a review from a team as a code owner June 2, 2020 22:56
@nebhale nebhale requested review from a team June 2, 2020 22:56
@ekcasey
Copy link
Member

ekcasey commented Jun 3, 2020

I really like this idea overall, but I think the big outstanding question here is the UX for direct=false processes. When a direct=false process is launched with a shell, the process arguments are actually arguments to bash -c <cmd> To make this work, each direct=false command would need to explicitly account for the possibility of arguments to take advantage of this feature.

As a simple/contrived example, imagine "hello world" type process.

A directly executed version of this command would look like the following:

[[processes]]
  type = "hello"
  command = "echo"
  args = ["hello"]
  direct = true

If we implemented this RFC and I passed "world" to the launcher the command essentially becomes

[[processes]]
  type = "hello"
  command = "echo"
  args = ["hello", "world"]
  direct = true

what gets executed by the launcher is essentially just echo hello world. Everything works great.

BUT a direct=false hello process could easily look like:

[[processes]]
  type = "hello"
  command = "echo hello"
  direct = false

Passing the same "world" argument to the launcher would result in

[[processes]]
  type = "hello"
  command = "echo hello"
  args = ["world"]
  direct = false

and the launcher would essentially execute bash -c "echo hello" "world". This doesn't work.
The command itself would have to be carefully constructed to accept bash arguments intuitively:

[[processes]]
  type = "hello"
  command = "echo $@"
  args = ["echo", "hello"]
  direct = false

AND even the above doesn't work in practice when read from a config.toml file. After experimenting with every permutation of single and double quoting the only thing i could get to work is toml unicode escape sequences for the $@ so it is evaluated at the correct time...

[[processes]]
type = "hello"
command = "echo \u0024\u0040"
args = ["echo", "hello"]
direct = false
EOF

(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. /cnb/lifecycle/launcher 'echo $@' echo hello world works so IDK why it's so hard to recreate the same behavior when reading from config.toml...)

If we fix quoting edge cases this UX might be okay? Folks that want to construct direct=false processes that accept args have a mechanism, but they will need to be careful and I imagine this could trick a lot of people.

@nebhale
Copy link
Contributor Author

nebhale commented Jun 3, 2020

@ekcasey I'd go with something significantly simpler, appending the arguments, space delimited, to the command itself.

text/0000-arguments.md Outdated Show resolved Hide resolved
@ekcasey
Copy link
Member

ekcasey commented Jun 3, 2020

@nebhale appending space delimited arguments to the command was an idea I was interested in for direct=false process args, even when they are in original process definition and not dynamically supplied (it would resolve the confusion described in buildpacks/lifecycle#281). I know @sclevine had some concerns about that idea however, on which I am sure he will be happy to elaborate.

@hone
Copy link
Member

hone commented Jun 3, 2020

I'm 👍 on this idea as well, but pending resolving the direct=false case.

@sclevine
Copy link
Member

sclevine commented Jun 3, 2020

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 direct = false and non-empty args?

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?

@natalieparellano
Copy link
Member

@ekcasey in the example you provide:

[[processes]]
  type = "hello"
  command = "echo $@"
  args = ["echo", "hello"]
  direct = false

Do you mean to say args = ["hello", "world"]? Just trying to follow along here, want to make sure I'm not missing something.

@sclevine
Copy link
Member

sclevine commented Jun 3, 2020

Request for prior art: what other tools solve this problem?

@jabrown85
Copy link
Contributor

https://docs.npmjs.com/cli/run-script#description allows arguments via --. Looks like the parsing is described as platform dependent.

@jabrown85
Copy link
Contributor

+1 to this idea. I've been wanting to be able to define npm install task that takes arbitrary arguments for a development mode.

ekcasey and others added 2 commits June 5, 2020 18:50
* 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]>
text/0000-arguments.md Outdated Show resolved Hide resolved
@nebhale nebhale requested a review from hone June 6, 2020 14:38
text/0000-arguments.md Outdated Show resolved Hide resolved
text/0000-arguments.md Outdated Show resolved Hide resolved
text/0000-arguments.md Outdated Show resolved Hide resolved
text/0000-arguments.md Outdated Show resolved Hide resolved
text/0000-arguments.md Outdated Show resolved Hide resolved
text/0000-arguments.md Outdated Show resolved Hide resolved
text/0000-arguments.md Outdated Show resolved Hide resolved
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:
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 you meant did here.

Suggested change
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:

Copy link
Member

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`.
Copy link
Contributor

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.

Copy link
Member

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"`.
Copy link
Contributor

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?

Copy link
Member

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]>
@nebhale
Copy link
Contributor Author

nebhale commented Jun 19, 2020

Final Comment Period with merge disposition, closing on 26 June, 2020.

@dfreilich
Copy link
Member

Simple question, but to make sure I understand:
With this RFC, if I would build an image of pack using pack (e.g. pack build pack -B gcr.io/paketo-buildpacks/builder:tiny -p .), I would be able to use it just by calling

docker run --rm -it pack version

rather than what we currently have to do (docker run --rm -it pack -- pack version)?

@jabrown85
Copy link
Contributor

Simple question, but to make sure I understand:
With this RFC, if I would build an image of pack using pack (e.g. pack build pack -B gcr.io/paketo-buildpacks/builder:tiny -p .), I would be able to use it just by calling

docker run --rm -it pack version

rather than what we currently have to do (docker run --rm -it pack -- pack version)?

Assuming the built image has a pack process type, yes. The pack launcher process would become the entrypoint and the version would be the argument.

@jromero
Copy link
Member

jromero commented Jun 25, 2020

@ekcasey / @nebhale can we rename the PR title and the RFC name to reflect that this is talking "image launch flags"

@ekcasey ekcasey changed the title Lifecycle Arguments Launcher Arguments Jun 25, 2020
nebhale added a commit that referenced this pull request Jun 26, 2020
[#84]

Signed-off-by: Ben Hale <[email protected]>
@nebhale nebhale merged commit 03d7eca into main Jun 26, 2020
@nebhale nebhale deleted the arguments branch June 26, 2020 20:04
jromero added a commit to jromero/rfcs that referenced this pull request Jun 18, 2021
jromero added a commit to jromero/rfcs that referenced this pull request Jun 18, 2021
jromero added a commit to jromero/rfcs that referenced this pull request Jun 18, 2021
jromero added a commit to jromero/rfcs that referenced this pull request Jun 18, 2021
jromero added a commit to jromero/rfcs that referenced this pull request Jun 18, 2021
jromero added a commit to jromero/rfcs that referenced this pull request Jun 18, 2021
jromero added a commit to jromero/rfcs that referenced this pull request Jun 18, 2021
jromero added a commit to jromero/rfcs that referenced this pull request Jul 13, 2021
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]>
jromero added a commit that referenced this pull request Jul 28, 2021
haliliceylan pushed a commit to haliliceylan/rfcs that referenced this pull request Aug 20, 2021
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]>
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.