Update to libcnb 0.12.0 and Buildpack API 0.9 #150
Conversation
libcnb 0.12.0 includes an upgrade from Buildpack APi 0.8 to 0.9: https://github.com/heroku/libcnb.rs/releases/tag/v0.12.0 In buildpack API 0.9 implicit usage of bash via `direct=false` mode has been removed: https://github.com/buildpacks/spec/releases/tag/buildpack%2Fv0.9 https://github.com/buildpacks/rfcs/blob/main/text/0093-remove-shell-processes.md As such, this buildpack now needs to explicitly wrap the procfile commands in a `bash -c "exec <command>"` invocation, similar to what the Python CNB already does for Python Functions: https://github.com/heroku/buildpacks-python/blob/76795e93cac436724b0f854c22b8df058418e728/src/salesforce_functions.rs#L57-L76 Closes #147. Closes #148. Closes #149. GUS-W-13119392.
|
I'm unsure about the use of This is not necessarily a bug and could be a feature, have you any thoughts around that? |
src/launch.rs
Outdated
| args: Vec::<String>::new(), | ||
| direct: false, | ||
| command: vec![String::from("bash")], | ||
| args: vec![String::from("-c"), format!("exec {value}")], |
There was a problem hiding this comment.
Thinking about this more, I'm not sure we need the exec here, and in fact I wonder if it would break with procfile entries like foo && bar, since the command would end up being exec foo && bar, with the exec potentially only applying to foo.
There was a problem hiding this comment.
Summoning our bash wizard @dzuelke. 🪄
Ah true. And in fact, in the RFC the example migrations they give (I've only just spotted these) use |
Which demonstrates the issues with prefixing the command with `exec`.
Since: - for single command procfile entires, using `bash -c <command>` already implicitly uses exec (making `exec` redundant) - for more complex compound command procfile entries exec should not be used otherwise later commands won't run (eg only `foo` would get run in `exec foo && bar`)
|
I'll explore the |
|
I'm closing this out for now since I discovered some additional complications around switching to Short version:
Long version:
|
|
I've filed #151 to track migrating away from |
The `Procfile` format supports specifying compound bash commands like: ``` web: foo && bar ``` This buildpack currently supports these, however, there is no integration test coverage of them. Having coverage is important, since some of the approaches for implementing the `bash -c` wrapping don't support usage of compound commands, as seen in: #150 (comment) As such I've added a new integration test for this, which also tests quoting (in case a future buildpack implementation starts trying to parse/escape the commands) and variable interpolation. This test case was extracted from #150 (which itself was superseded).
libcnb 0.12.0 includes an upgrade from Buildpack APi 0.8 to 0.9:
https://github.com/heroku/libcnb.rs/releases/tag/v0.12.0
In buildpack API 0.9 implicit usage of bash via
direct=falsemode has been removed:https://github.com/buildpacks/spec/releases/tag/buildpack%2Fv0.9
https://github.com/buildpacks/rfcs/blob/main/text/0093-remove-shell-processes.md
As such, this buildpack now needs to explicitly wrap the procfile commands in a
bash -c "<command>"invocation, similar to what the Python CNB already does for Python Functions:https://github.com/heroku/buildpacks-python/blob/76795e93cac436724b0f854c22b8df058418e728/src/salesforce_functions.rs#L57-L76
We're not using
execfor the reasons mentioned in:382d96e
I've also added an integration test for more complex procfile entries (such as those that use compound commands and env var interpolation) - since the existing tests didn't catch the
execissue.See also:
https://buildpacks.io/docs/reference/spec/migration/buildpack-api-0.8-0.9/
https://buildpacks.io/docs/reference/spec/migration/platform-api-0.9-0.10/
https://github.com/buildpacks/spec/blob/main/buildpack.md#launchtoml-toml
https://github.com/buildpacks/spec/blob/main/platform.md#launcher
Closes #147.
Closes #148.
Closes #149.
GUS-W-13119392.