Override env vars #144
Override env vars #144
Conversation
buildpack.md
Outdated
| @@ -666,6 +666,23 @@ For each file written to `<layers>/<layer>/env.launch/` by `/bin/build`, the lif | |||
| The lifecycle MUST consider the name of the environment variable to be the name of the file up to the first period (`.`) or to the end of the name if no periods are present. | |||
| In all cases, file contents MUST NOT be evaluated by a shell or otherwise modified before inclusion in environment variable values. | |||
|
|
|||
| ##### Override | |||
|
|
|||
| If the environment variable file name has no period-delimited suffix, then the value of the environment variable MUST be the file contents. | |||
There was a problem hiding this comment.
What do you think about adding an examples area to clearly show the association between the file name and the definition of behavior? I think this would also have the benefit of hoisting up the no period-delimited behavior to make it clearer.
Examples
| Filename | Modification Behavior |
|---|---|
<layers>/<layer>/env/<env_var> |
Override |
<layers>/<layer>/env/<env_var>.override |
Override |
<layers>/<layer>/env/<env_var>.default |
Default |
<layers>/<layer>/env/<env_var>.delim |
Default |
<layers>/<layer>/env/<env_var>.prepend |
Default |
<layers>/<layer>/env/<env_var>.append |
Default |
There was a problem hiding this comment.
Good idea. I updated the branch to move all of the suffix information into a table at the top. With that change I thought it made sense to alphabetize the modification types (instead of putting the default first).
There was a problem hiding this comment.
Are we still going to have a .override? We're already breaking compatibility, and I'm not a fan of having two ways to accomplish the same behavior.
There was a problem hiding this comment.
By using .override or .prepend explicitly buildpack authors can currently write buildpacks that will not break when the API changes. This would not be true if we removed .override in buildpack API 0.5.
Also, removing .override would require more buildpacks to update to accomodate this change.
In my mind, the above reasons outweigh the cost of having two ways to accomplish the same behavior. Personally I also like the symmetry of having a suffix for every operation (but that probably not a very good reason).
ekcasey
force-pushed
the
override-env-vars
branch
2 times, most recently
from
4de1c3e
to
7a76cb6
Compare
buildpack.md
Outdated
| - For default environment variable file contents originating from the same buildpack, file contents that are earlier (when sorted alphabetically ascending by associated layer name) MUST override file contents that are later. | ||
| - **Default environment variable file contents originating in the same layer MUST be sorted such that file contents in `<layers>/<layer>/env/` override file contents in `<layers>/<layer>/env.build/` or `<layers>/<layer>/env.launch/` which override file contents in `<layers>/<layer>/env.launch/<process>/`.** | ||
| The value of the environment variable MUST be a concatenation of the file contents and the contents of other files representing that environment variable. | ||
| In either case, within that environment variable value, |
There was a problem hiding this comment.
I'm a bit confused - what are the two cases here?
There was a problem hiding this comment.
Good catch, this is a remnant from a time when this line was describing either .prepend or no suffix cases.
|
I have a general question: |
|
Do we need to cover how buildpack API |
Signed-off-by: Emily Casey <[email protected]>
* Alphatbetize modification types * Improves readability/navigation Signed-off-by: Emily Casey <[email protected]>
Signed-off-by: Emily Casey <[email protected]> Co-authored-by: Natalie Arellano <[email protected]>
Signed-off-by: Emily Casey <[email protected]>
ekcasey
force-pushed
the
override-env-vars
branch
from
62ede8e
to
1ac3fd7
Compare
Outcome from leadership meeting discussion: This belongs in lifecycle documentation rather than the spec given that another compliant lifecycle implementation may reasonably choose to support only API 0.5. Also, the lifecycle implementation provided by the CNB project currently supports any combination of buildpacks with APIs >= to 0.2 in a single group. Therefore the above statement about compatibility is not specific to this change but rather is true generally. |
reorders modification types so that the default behavior is easy to find (at the top)