RFC: Change environment variable delimiter default value

[sgaist]

Related to buildpacks/spec#285

[buildpack-bot]

Maintainers,

As you review this RFC please queue up issues to be created using the following commands:

/queue-issue <repo> "<title>" [labels]...
/unqueue-issue <uid>

Issues

(none)

[dmikusa]

What about changing the default for specific env variables? There are well-known env variables where what you're saying is true, like PATH and LD_LIBRARY_PATH, but there are other well-known env variables where space separated is used, like JAVA_TOOL_OPTIONS.

Or what about just leaving this as a choice for build image maintainers? I believe they can use build config to override this value.

I agree that an empty delimiter isn't helpful pretty much ever, but I don't agree that path separator is what you want the majority of the time, and I feel like the default should reflect the most commonly used option. I could almost see the current default value being picked because it would be difficult to pick something that works for most cases, so picking empty string is a neutral choice.

[edmorley]

I did quite a bit of research in buildpacks/spec#285 (comment) which found that:

1. The : delimiter was the most commonly used
2. No CNBs used the current empty string default delimeter
3. A small number of delimiter usages were for a single space delimeter

The current default of the empty string is therefore not helpful, and a footgun (hence #285 being filed).

I feel that trying to have a magic list of env vars that get given a : delimiter would be confusing for end users, and something that would be virtually impossible to keep up to date with all of the ecosystems env var usages.

Therefore as mentioned in #285 I feel we should either:

1. Have the PATH env var as the default (which basically means :, given Windows support is deprecated/sunset)
2. Have no default, and error if no delimiter is set.

I'm currently leaning towards (1), which is what this RFC proposes.

[dmikusa]

If this were all new, and there were no existing buildpacks, I would have no concerns about what you're proposing. The problem is that the buildpack spec is not new, and there are hundreds of existing buildpacks writing to it. I know that buildpack spec is not 1.0, so things can change, but IMHO, we missed the window to change something like this. The reason I think we missed the window is that this seems like a trivial change. I don't see a major advantage to changing this. Unless I'm missing something, and please correct me if I am, this is a minor ergonomic win for new buildpack authors. But that is at the expense of inconveniencing all the existing buildpack authors who have solved this already.

I would be in favor of either a.) doing nothing with the spec and documenting the state of things (i.e. improve the documentation on the existing behavior, thereby making things easier on new buildpack authors), or b.) having a way that it can be set through the build image, like with build config. That would at least give more control over how this works to buildpack authors. Buildpack authors could essentially opt out of the change if it is a nuisance for them, but it would still allow them to keep up-to-date with the most recent buildpack APIs. I am concerned about that last point with Paketo, and I see that as a potential road block for updating API versions.

[schneems]

It's also worth mentioning that the current spec https://github.com/buildpacks/spec/blob/d29ba81518b7304700e9e8a54cc2194fa732e51b/buildpack.md doesn't actually state what the default should be. The only mention of an explicit value (that I can grep for), explicitly mentions OS path separators:

For layer path environment variables, user-provided values are prepended before any existing values and are delimited by the OS path list separator.

Responding to suggestions:

I would be in favor of either a.) doing nothing with the spec and documenting the state of things (i.e. improve the documentation on the existing behavior, thereby making things easier on new buildpack authors),

I don't think it's a visibility issue. I raised the original issue on github, and even still, I hit this from time to time. Recently even. This default has collectively lost me more development hours than any other feature in the CNB spec. If the API is a danger zone, and we cannot provide a good default, then we should force the user to be explicit. I would rather we make an empty delimiter a hard error than do nothing.

I want it to be easier to do the right thing (and harder to do the wrong thing) than writing a Dockerfile.

having a way that it can be set through the build image, like with build config.

Same feedback. If we did this, I would want to make it a required value. Hard error if it's not there.

we missed the window to change something like this.

Buildpacks opt into versions of the spec, so we have a way to gate this change.

We also have a way to detect when someone is currently relying on the missing delimiter. We could have the lifecycle detect and emit a warning in that case:

WARNING: Buildpack `heroku/ruby` is using the default PATH delimiter of `` (empty string). This value will change to an OS specified path separator in spec version `0.X`. If this buildpack depends on this behavior, they need to write an explicit empty string as the `PATH.delim`.

I would guess the vast majority of warnings will be output to apps that are unaware that they're appending/prepending using an empty string as a delimiter.

I see this change as a fairly small (but meaningful) usability improvement. A possible other path forward would be asking the question "what features or infrastructure would let us confidently make this change"? I.e. there could be an intermediate feature like pack build --fail-on-warning or something or pack build --lint that we could encourage developers to add to their CI where we could raise problems.

Recap:

• I'm 👎 to doing nothing
• I'm 👎 to making it a build config, but would change that to luke-warm if we can make it a required build config.
• Still think we should change the default, emitting a deprecation warning would be the safest way forward
• Possibly explore other tools/tooling that could let us iterate

[dmikusa]

A possible other path forward would be asking the question "what features or infrastructure would let us confidently make this change"? I.e. there could be an intermediate feature like pack build --fail-on-warning or something or pack build --lint that we could encourage developers to add to their CI where we could raise problems.

Anything that would not require changes to existing buildpacks or the command required to do builds.

As a Paketo dev, I can tell you with a pretty high degree of certainty that no one is going to go update our buildpacks to make this change. There's no benefit to the change, so it's 100% busy work. With only a small group of volunteers, this is the kind of change for an OSS project that never gets done.

I can't speak for all companies, but for those same reasons, it's the kind of change that is super annoying as well. I'm using buildpacks as a tool to without much or any effort build my container images. If the upstream project is breaking my builds or requiring me to change things and get no benefit, it's quite frustrating.

If you change the default, but provide a convenient way to retain the prior behavior, then this becomes moot. Those who want to can opt-into keeping the previous behavior can, but the project can update the defaults to something more sensible.

I get why you want to make this change, and I think the change is a good improvement from the point of view of a developer trying to learn about buildpacks. I just don't like the way that it impacts existing buildpack users.

[dmikusa]

This default has collectively lost me more development hours than any other feature in the CNB spec.

Total tangent, but the buildplan is a way more confusing part of the spec. :)

[dmikusa]

Paketo has an environment-variables buildpack that allows users to embed environment variables into their app images. This exposes the append & delimiter functionality. We have no way of knowing what users are setting and if they depend on the default value. Making this change should be considered disruptive for end users and not just buildpack authors.

[edmorley]

The Paketo buildpack could work around this though? eg It could set a default delimiter of the empty string if it wanted, or emit a warning, or even error if no user-provided delimeter is set.

[dmikusa]

Possibly. I haven't thought through it tbh. I think if we had a way to change this globally, like at the build image level (see my other comment), then I would agree with your assessment of the impact. Otherwise, I feel the opposite. That the small benefit of making this change for new buildpack authors doesn't outweigh the risk/impact of making the change. Again, I may not be seeing the full benefits of this change, so if I'm missing something, please let me know.

[edmorley]

I don't think the benefit of the change is small though, and I disagree that there is a big impact/risk of making the change - and have provided evidence as to why, that hasn't been countered :-)

IMO it's important for us to remove any footguns new users writing buildpacks might encounter - and this rough edge is something multiple people have run into. There's also no point in us having a versioned Buildpack API, if we're not willing to use this versioning to make carefully thought out (and low impact) changes to improve the spec from time to time :-)

[dmikusa]

That's fine, we can agree to disagree then as it seems we just have differing opinions here. Personally, I would rather see us move to a stable release than continue to tweak things like this that just create busy work for existing buildpack developers and users of those buildpacks. That's just my $0.02 though. I respect that we might not all agree, that's life, and whatever the group decides to do, is fine by me.

[schneems]

As mentioned above, this is a major pain point for me. I think people should still be able to append/prepend with an empty string if they want, but then they should be using an explicit delimiter (similar to how the rest of us are forced to use an explicit one—speaking of toil).

I suggested a deprecation cycle as a way to help make the reliance on this silent behavior observable. If we add a warning in, and hear a large outcry from lots of buildpack authors, then we'll have more feedback and don't have to guess.

[dmikusa]

I suggested a deprecation cycle as a way to help make the reliance on this silent behavior observable. If we add a warning in, and hear a large outcry from lots of buildpack authors, then we'll have more feedback and don't have to guess.

I used to think that putting a warning into the output of the build command was a good way to notify users about changes, but I think that's a pack-centric view of things. The build output is hidden from a lot of users and they simply don't see it.

If you have builds set up in your CI pipeline, you're not going to see any warning in the build output, cause you won't even go look unless something fails. Other platforms that wrap buildpacks are the same way. You just don't see that output unless something fails.

[jabrown85]

@hone would you mind stewarding this one through?

[jkutner]

@sgaist we discussed this in our last working group session, and I'm afraid we've decided that the backwards-incompatibility, or breakage of existing buildpacks, is too significant for us to move this forward. We'd be happy to discuss some alternatives, but we think it's important not to break existing buildpacks.

[edmorley]

@jkutner Hi! Did this discussion take into account that my analysis showed no buildpacks in the wild that would be affected?
#326 (comment)

I really do think we should proceed with this change in some form - even if it's to output an explicit error in the "no delimiter specified" case (which would help with any concern about people missing this in the specification release notes).

[schneems]

Stated previously but to reiterate if it got lost in the fold: I would rather the absence of this file raise an error that is opt-out instead of the current behavior. That's how strongly I believe this API is broken.

[edmorley]

@jkutner Please can this be reopened. I don't feel it was rejected in a fair way (since it occurred in a sync meeting without our involvement, rather than as a discussion on this async and open PR) - and we've also had no confirmation that the data I provided against one of the objections here was even discussed at the meeting.

[schneems]

I’ve got an in progress (not submitted) PR to issue a warning from lifecycle when this condition happens. We can pair that with a (yet to be proposed) pack feature that exits non-zero on any warning to effectively make this behavior an error. Alternatively we make a new RFC to make this footgun into a proper error.