Add POSIX default value support on vars

[ulyssessouza]

This PR considers POSIX default values on variables with this, the following tests are now possible:

# Undefined with default value
({}, "a=${b:-DEFAULT_VALUE}", False, {"a": "${b:-DEFAULT_VALUE}"}),
({}, "a=${b:-DEFAULT_VALUE}", True, {"a": "DEFAULT_VALUE"}),
({"b": "c"}, "a=${b:-DEFAULT_VALUE}", False, {"a": "${b:-DEFAULT_VALUE}"}),
({"b": "c"}, "a=${b:-DEFAULT_VALUE}", True, {"a": "c"}),

Note that this PR depends on #241 being merged before to pass the CI

[coveralls]

Coverage increased (+0.2%) to 90.215% when pulling a82faf9 on ulyssessouza:add-posix-default-value-support into 12439aa on theskumar:master.

[ulyssessouza]

Note this is also a fix. Since the interpolation is set to True.
$b and ${b} should have the same behavior.

[ulyssessouza]

@bbc2 Could you review that pls?

[ulyssessouza]

@bbc2 Any prevision on when you can review that?
That's the only thing missing to release docker-compose 1.26.0

[bbc2]

I've started looking at this and I like the idea of extending the current expansion of variables but I need a more time.

I would like to ensure that interpolating variables without the braces is OK. We used to explicitly require the braces (${foo}). That made parsing easier, and also allowed users to use literal $ characters in their variables.

Also, I'll need to double-check that but I think that there's a problem when a variable is not at the beginning of the value.

[ulyssessouza]

Thank you for having a look!
Feel free to push any changes that you consider as needed when you have the time.

[thaJeztah]

@bbc2 ptal 🤗

[bbc2]

Sorry for not having updated you earlier on the status of this.

From my look into this MR, I am not comfortable with the following things:

• The breaking of variable expansion when there are characters before the variable (e.g a=b${c}).
• The expansion of variables without braces (e.g $b). While this might be desirable in the future, I would like to avoid dealing with such a breaking change right now because I would expect many .env files to contain bindings such as PASSWORD=rN/I3+d$M5I. However, I would welcome your thoughts on this.
• The use of the sophisticated parsing mechanism outside of the parser module. It is very powerful but tricky to use, as you may have seen.

I considered moving all the parsing to the parser module, but that would have required too much time. Eventually, I realized that the expansion of default values could be achieved with a reasonably simple change to the __posix_variable regex. Since it's a new approach to the problem, I created a new MR: #248.

Does it solve the default value problem for you?

[ulyssessouza]

@bbc2 I agree with the first and third items.

About the second, I see as a bug because the same example PASSWORD=rN/I3+d$M5I renders as rN/I3+d in a shell. To render as rN/I3+d$M5I the $ must be escaped, like in PASSWORD=rN/I3+d\$M5I.

[bbc2]

What you say about the shell is correct, and I think that it would be sensible for Python-dotenv to see a string like $foo as a variable. My point is rather that I would prefer to think more about that change before making it, as it is likely to be breaking for many users.

If you're OK with the default values as implemented in #248, I can merge that and release a new version right away, since it's a harmless change.

In any case, I'll think more about the interpretation of variables without braces like $foo this week-end and hopefully release that a bit later. For instance, I think it would make sense to stop expanding variables in single-quoted values. That would reduce the impact of the change by not breaking bindings like PASSWORD='ab$fd+'. The handling of escaped $ would also need some care. For instance, a value such as \${foo} is expanded in the current implementation, which is unlike what happens in a shell.

[thaJeztah]

Yes, dealing with these using regular expressions may be difficult. I've worked on a PR a while back to perform variable substitution (for Docker build), and in that code we're not scanning the input (I didn't write the original code, but IIRC it was because regular expressions didn't really work); https://github.com/moby/moby/pull/37134/files

There's also "special" characters that could be taken into account (we don't support those in Dockerfiles, because there's no real "environment", but perhaps in this project they could be relevant); http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_02

[ulyssessouza]

Closing in favor of #248

[bbc2]

Yes, dealing with these using regular expressions may be difficult. I've worked on a PR a while back to perform variable substitution (for Docker build), and in that code we're not scanning the input (I didn't write the original code, but IIRC it was because regular expressions didn't really work);

Yes, I agree.

One of my first contributions to this project consisted in replacing the whole parser by a single regular expression. It did fix a number of bugs, but it also prevented some new features from being implemented, and so I replaced it with a classic top-down parser.

The regular expression used to parse the variables existed long before all that, and hasn't been replaced by anything else since then. I think it would make sense to handle the parsing of variables in the parser, but that will require some time.

https://github.com/moby/moby/pull/37134/files

That test suite looks very interesting. Thanks!

There's also "special" characters that could be taken into account (we don't support those in Dockerfiles, because there's no real "environment", but perhaps in this project they could be relevant); http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_02

OK, so implementing this would probably be a stretch.

If I understand correctly, this project started as a way to facilitate the development of 12-factor apps (because you then wouldn't need to wrap your app in a Bash script). Under this methodology, what you pass via the environment is supposed to be simple (some flags and strings), and complexity like default values and derived values should be in the app itself, not the environment.

That doesn't mean we can't be beneficial to other users, but I don't think we'll ever fully emulate Bash. That would be so much work.

Anyway, the new version is out! 🎉

[thaJeztah]

That test suite looks very interesting. Thanks!

Can't take credits for that (someone else implemented it); it's a bit "creative", but there's some good (corner-case) examples in there.

If I understand correctly, this project started as a way to facilitate the development of 12-factor apps (because you then wouldn't need to wrap your app in a Bash script). Under this methodology, what you pass via the environment is supposed to be simple (some flags and strings), and complexity like default values and derived values should be in the app itself, not the environment.

(disclaimer: I'm mainly passing-by; I'm not a Python developer, other than having made some minor contributions to docker-compose). Thanks for that background, and yes, that makes sense.

I also agree that "supposed to be simple" is important; feature creep has been a constant struggle for us with docker and docker-compose as well: Just because we can implement something, doesn't mean we should - even more so if there's a solution already (which could be "use a .bashrc"), hence my earlier comment #242 (comment) (I marked it as "off topic").