Bug when prepending to PATH twice on the same layer

[schneems]

Expected

If I prepend PATH with b and then prepend it with a that the outcome will be PATH=b:a:$PATH.

Actual

Only the last modification is preserved, it ends up being PATH=a:$PATH.

Repro

$ cargo new env_delimiter_apply
$ cd env_delimiter_apply
$ cargo add libcnb
$ cargo add pretty_assertions

Stick this code in your main.rs and run tests:

$ cargo test
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.06s
     Running unittests src/main.rs (target/debug/deps/env_delimiter_apply-487764a010d26946)

running 2 tests
test test::test_simple_path ... ok
test test::test_double_path ... FAILED

failures:

---- test::test_double_path stdout ----
thread 'test::test_double_path' panicked at src/main.rs:46:9:
assertion failed: `(left == right)`

Diff < left / right > :
<PATH=b:a
>PATH=a

[colincasey]

I've been hit by this as well (see #790). My workaround is here.

[edmorley]

So I agree this is confusing, but it's currently working as designed.

libcnb.rs is a thin layer over the underlying CNB spec. The CNB spec doesn't have a way to say "I want to set multiple values for this delimited field, please concatenate them with the delimiter for me".

We could of course change the libcnb.rs API here (there are other parts of libcnb that try and smooth over usability issues in the underlying spec), though it may end up meaning we have to complicate the API or make things confusing in other ways.

[schneems]

I disagree that the spec upstream has anything to say about the affordance over what goes into the output. i.e. it doesn't say if you should use it like:

# Replace
echo bar > PATH.prepend
echo foo > PATH.prepend

Or

# Prepend
echo "foo:$(cat PATH.prepend)" > PATH.prepend

But also, whether we agree or disagree on this point is a bit of a tangent to my overall goals.

We can change this behavior today without changing the current API. The only problem (i can forsee) would be is it would be a breaking change for anyone using the prepend modification behavior to indicate "replace this" or "delete/clear" this.

At a high level, my position is that any interface that looks like it works but confuses experienced users is a bug. This interface isn't intuitive as-is. The big question becomes, what should the interface be?

When designing for an API, I need to consider all the use cases; I also like to know what the most common use cases are so I can optimize for those. Also, consider unusual/unexpected use cases and make sure that they're still possible even if less ergonomic. For example, someone might use this current interface to write a prepend of "" to effectively intend to delete the contents. If we were to modify the existing function, I would suggest detecting this scenario and emitting a warning.

I'm interested in brainstorming/prototyping interfaces. We can even try one out in our own buildpacks before committing to something upstream.

[edmorley]

I agree the current design is confused (as mentioned above) - I was just explaining the current state. We can of course change anything we want, but the first step is understanding the current design/implementation.

(As a heads up I won't have time to work on/discuss this or anything else non-urgent-Python until after PyCon, given time constraints)

[schneems]

In researching this, the original design is described as a "higher level" API for environment variables https://github.com/heroku/libcnb.rs/pull/30. I'm going to do an audit on all the places prepend is called in our existing buildpacks and determine if modifying the behavior in place (to "insert" the prepend rather than replace it) would break anyone. As a new API is going to inevitably be a bikeshed.

• [ ] .NET
• [ ] Go
• [ ] Java (Gradle)
• [ ] Java (Maven)
• [ ] Node.JS
• [ ] PHP
• [ ] Python
• [ ] Ruby
• [ ] Scala
• [ ] .deb packages
• [ ] Procfile
• [ ] Others???