-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add support for process launch env. vars. #127
Conversation
Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
Hi @samj1912 thank you for the contribution! IIRC, part of the reason #70 was closed out (we should've documented this better on the PR) was because we didn't want to force layer creation for processes just to allow for environment variables to work. |
@sophiewigmore IIUC the spec doesn't allow for you to set process specific environment variables without creating a layer. This PR was supposed to simply match packit functionality with the spec. I know this is not ideal but I think there is very little we can do about it right now. This also goes in line with the other env variables that can be set in the layer struct. I think we should create an upstream issue to allow users to specify env vars for a process without creating an additional layer, but I think meanwhile we should merge this so that atleast we are providing appropriate bindings for the spec. Also IIUC this PR might be different from the previous PR which explicitly added an additional layer for storing process env vars. Instead, this allows you to use an existing layer or create a new one based on your needs. Lmk what you think :) |
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.
This looks like a reasonable implementation of the current spec. The spec's implementation is unfortunate, but we should still at least support what is currently specified. Thank you @samj1912 for taking the time to implement this.
Signed-off-by: Sambhav Kothari skothari44@bloomberg.net
Summary
Adds support for process specific launch env vars.
Fixes #63
Use Cases
To allow setting process specific env vars on launch
I see this was previously worked upon in #70
I think it might be simpler to just expose it in the layer instead of in the process struct. This goes well with the rest of env vars that are currently housed in layers and also if someone wants to create a separate layer just for their process - that should work fine. It also allows buildpacks to supply launch env. vars for processes not defined by them.
Checklist