Improve ergonomics for Append/Prepend use cases in LayerEnv API

[colincasey]

The API design of LayerEnv is meant to map directly to the environment variable modification rules of the CNB spec.

For many cases this API is suitable. But, when it comes to environment variables that fall under the Append or Prepend modification behavior, this API is less than ideal and can cause confusion for buildpack authors. Here are two such examples:

1. A buildpack author wants to append to the FOO environment variable and calls

   env.insert(Scope::All, ModificationBehavior::Append, "FOO", "some value");

   Without realizing they must also set a delimiter using or a default empty string will be used as a delimiter when the environment values are processed:

   env.insert(Scope::All, ModificationBehavior::Delimiter, "FOO", "-");

   This is a known issue and Document the need to set an explicit delimiter when using ModificationBehavior::{Append,Prepend} #700 proposes better documentation to help. It seems that the API could also assist buildpack authors here by providing methods to ensure a delimiter is set at the same scope whenever Append or Prepend modification behaviors are used since these two concerns are linked.

2. A buildpack author wants to prepend multiple values to the BAR environment variable and calls

   env.insert(Scope::All, ModificationBehavior::Delimiter, "BAR", ":");
   for value in values {
       env.insert(Scope::All, ModificationBehavior::Prepend, "BAR", value);
   }

   Due to the semantics of Append and Prepend, I think it's easy to misinterpret what env.insert is doing here and expect that it would modify BAR to insert each value using a prepend operation with the required delimiter. What it actually does is overwrite the value of BAR on each iteration so only the last value is persisted. This makes sense at a low-level but, like the required delimiter issue above, does no favors for the buildpack author. To get the correct result the delimiter needs to be used twice as can be seen in the following code:

   env.insert(Scope::All, ModificationBehavior::Delimiter, "BAR", ":");
   env.insert(
       Scope::All,
       ModificationBehavior::Prepend,
       name,
       values.join(":"),
   );

Since append and prepend semantics are typically associated with list operations, it would be preferable if the LayerEnv API matched those expectations when ModificationBehavior::{Append,Prepend} operations are involved and would:

• enforce the delimiter requirement
• allow for multiple values to be provided

[Malax]

Thanks for the write-up!

I'm worried when we strife too far from the CNB spec that it will lead to hard reason about where certain behaviour comes from. If someone runs into the issue with a missing delimiter, they can search for CNB in general and will get an answer that will point them to the solution they can also use with libcnb.rs since the API is very close to the spec.

allow for multiple values to be provided

That's another issue with the spec as it uses one file per environment variable. You can put multiple values in that file, but you'd need to concatenate with some delimiter before. There is no way to use the same delimiter mechanism from lifecycle implicitly.

The whole idea of encoding the environment modifications as file system trees causes a lot of issues and makes it hard to work with. Using a TOML file would make it easy to enforce (in lifecycle) a delimiter with each append/prepend operation. One can argue that the current file system approach it's an easy interface for scripting languages, but buildpack authors have to write TOML in many cases anyway. Moving this feature to TOML as well would be more up- than downside, but that's a discussion for upstream.

TL;DR: This is much more complicated to get right as it might seem. Playing it safe and keeping close the spec is my preferred solution mid-term. Working with upstream on the spec in that area sounds more fruitful to me.

[colincasey]

👍 Still wanted to capture it here. Improving the situation with the upstream spec is definitely the ideal so maybe LayerEnv is the wrong place for this suggestion since you want to keep that in sync with the spec.

Would a utility function in libherokubuildpack be more agreeable? Something at least to help buildpack authors avoid this footgun?

[Malax]

As for stopgap solutions I think upstream is again our best bet.

Ideally lifecycle would log something akin to "layer XYZ of buildpack ABC uses append/prepend for env var FOO without a delimiter" when no delimiter is set. I can't think of many cases where no delimiter is actually useful and intended. And if there are some, writing an empty file to make this explicit (to silence the proposed lifecycle warning) seems preferable.

Not sure how a helper in libherokubuildpack would look like, do you have anything in mind? Generally, things in libherokubuildpack can be more lenient and/or specific than in libcnb I would say.

[colincasey]

Just something simple that explicitly ties the two together like:

// append a single value to the env while ensuring that there is a delimiter set
fn append_value_to_env<T>(name: T, value: T, delimiter: T, scope: Scope)
where
    T: Into<OsString>;

// append multiple values to the env while ensuring that there is a delimiter set
// and each value is joined using that delimiter
fn append_values_to_env<I, T>(name: T, values: I, delimiter: T, scope: Scope)
where
    I: IntoIterator<Item = T>,
    T: Into<OsString>;

// prepend a single value to the env while ensuring that there is a delimiter set
fn prepend_value_to_env<T>(name: T, value: T, delimiter: T, scope: Scope)
where
    T: Into<OsString>;

// prepend multiple values to the env while ensuring that there is a delimiter set
// and each value is joined using that delimiter
fn prepend_values_to_env<I, T>(name: T, values: I, delimiter: T, scope: Scope)
where
    I: IntoIterator<Item = T>,
    T: Into<OsString>;