Skip to content

RFC: Change environment variable delimiter default value #326

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.

Sign up for GitHub

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

Open
sgaist wants to merge 1 commit into
base: main
Choose a base branch
from
Open

RFC: Change environment variable delimiter default value #326

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 106 additions & 0 deletions text/0000-change-env-var-delimiter-default-value.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
# Meta
[meta]: #meta
- Name: Change environment variable delimiter default value for append/prepend operations
- Start Date: 2025-03-12
- Author(s): sgaist
- Status: Draft <!-- Acceptable values: Draft, Approved, On Hold, Superseded -->
- RFC Pull Request: (leave blank)
- CNB Pull Request: (leave blank)
- CNB Issue: (leave blank)
- Supersedes: "N/A"

# Summary
[summary]: #summary

This RFC proposes to change the default empty delimiter value used in append and prepend operations when modifying environment variables to be the platform delimiter (e.g. `:` for Linux).

# Definitions
[definitions]: #definitions

N/A

# Motivation
[motivation]: #motivation

When appending or prepending a value to an environment variable such as PATH, the default delimiter used is an empty string. In the majority of cases, it's not the value required nor expected and buildpacks maintainers have to explicitly set its value to the separator used for the platform(s) they are targeting.
Copy link
Contributor

@dmikusa dmikusa Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@edmorley edmorley Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dmikusa dmikusa Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@schneems schneems May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@dmikusa dmikusa May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dmikusa dmikusa May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)


Changing that empty default value would simplify buildpack creation and maintenance because most of the time, appending or prepending a value requires a non empty delimiter.

As expected outcome, this will simplify the code of the buildpack and its content. Essentially removing the need for the `<env-var>.delimiter` file creation and its presence.

# What it is
[what-it-is]: #what-it-is

### Define the target persona:

Buildpack author, project contributor.

### Explaining the feature largely in terms of examples


### Migration

1) If the delimiter in use is the platform delimiter, remove the code that generates `<env-var>.delimiter`
2) If the delimiter in use is empty, add code to create an empty `<env-var>.delimiter` file

### Describe the differences between teaching this to existing authors and new authors.

For existing authors, they can drop the code handling the delimiter file creation if using the platform delimiter.

For new authors, the default new behaviour should be the expected beahaviour so there shouldn't be anything special to be done.

# How it Works
[how-it-works]: #how-it-works

The current default value should be changed to use the default platform separator.

# Migration
[migration]: #migration

As described above, only buildpacks authors that rely on an empty strings will have to take actions (i.e. create an empty `<env-var>.delimiter` file. Others can either keep the status, or cleanup their buildpack code.

# Drawbacks
[drawbacks]: #drawbacks

Currently, the benefits of simplifying the creation and maintenance of buildpacks outweighs the breakage of a small number of them.
Copy link
Contributor

@dmikusa dmikusa Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@edmorley edmorley Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dmikusa dmikusa Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@edmorley edmorley Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Copy link
Contributor

@dmikusa dmikusa Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@schneems schneems May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dmikusa dmikusa May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Alternatives
[alternatives]: #alternatives

N/A

# Prior Art
[prior-art]: #prior-art

N/A

# Unresolved Questions
[unresolved-questions]: #unresolved-questions

N/A

# Spec. Changes (OPTIONAL)
[spec-changes]: #spec-changes

- New default value of delimiter which was previously an empty string.

# History
[history]: #history

<!--
## Amended
### Meta
[meta-1]: #meta-1
- Name: (fill in the amendment name: Variable Rename)
- Start Date: (fill in today's date: YYYY-MM-DD)
- Author(s): (Github usernames)
- Amendment Pull Request: (leave blank)

### Summary

A brief description of the changes.

### Motivation

Why was this amendment necessary?
--->