Allow values in Pipfile to consume Environment Variables

[dvf]

Relying on os.path.expandvars() to inject environment variables in strings. Works on Windows, back-supports Python 2.7.

See #1688

I'm not 100% sure that this is the correct place to do this injection.

Some thoughts:

• The Pipfile hash should be determined after values have attempted to be inserted.
• Pipfile shouldn't be intelligent by trying to differentiate env vars from regular characters—if an env var can't be inserted, the string should remain intact.
• As it stands, Pipenv's vendor folder contains some version of this repository. I don't fully understand the reasons behind this, maybe we could pin it as a requirement instead? We could then separate tests.

Mirrored Pipfile PR: pypa/pipfile#105

[jtratner]

can you add a test that fails when an environment variable is not defined?

[jtratner]

(looks like it doesn't error just doesn't expand in python 3.6)

>>> os.path.expandvars('${A}')
'${A}'

[dvf]

This seems correct to me, would you expect it to throw an error?

…

On Mar 16, 2018, at 6:41 PM, Jeff Tratner ***@***.***> wrote: (looks like it doesn't error just doesn't expand in python 3.6) >>> os.path.expandvars('${A}') '${A}' — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[jtratner]

I'm concerned about something error about not connecting that turns out to be not being able to connect to $USERNAME:[email protected] or something like that.

Way nicer to error and say USERNAME not defined! :)

Also how does this impact hashing? Which parts of the pipfile are allowed to have env vars in them?

[jtratner]

(but I'm def not BDFL here :) )

[techalchemy]

I like the approach, but think this should be PR'ed to the pipfile repo as well- and any changes to packages in the vendor directory will require the package to move to the patched folder

[techalchemy]

Generally anything in the vendor directory needs to be kept pristine, any modifications would require this package to move into the patched directory

More significantly you should simultaneously PR this change back up to the pipfile repository as well since we are not the only project which uses pipfiles

[dvf]

I did that here: pypa/pipfile#105

[castarco]

Just for curiosity, which other projects are using Pipfiles?

[dvf]

After some thought I don't think this is ready. Would appreciate some clarity here.

Some thoughts:

• The Pipfile hash should not be affected by the values of the env vars, only the keys. This means the hash will have to be calculated before injection.
• Disagree with @jtratner regarding raising errors: Pipfile shouldn't be intelligent by trying to differentiate env vars from regular characters—if an env var can't be inserted, the string should remain the same.
• The reasons for having a vendor folder aren't clear to me if they haven't been patched: Why not just add Pipfile as a requirement?

[jtratner]

@techalchemy - maybe only the sources section should allow env vars? And then everywhere else it’s just interpreted literally? Or alternatively you add another key that are the vars to substitute? Generally, not sure where it would ever make sense to have `$` as a name. If hash is calculated before env var substitution then should it be disallowed in the packages sections?

…

On Fri, Mar 16, 2018 at 9:12 PM Daniel van Flymen ***@***.***> wrote: After some thought I don't think this is ready. Would appreciate some clarity here. Some thoughts: - The Pipfile hash should *not* be affected by the values of the env vars, only the keys. This means the hash will have to be calculated before injection. - *Disagree with @jtratner <https:/jtratner> regarding raising errors:* Pipfile shouldn't be intelligent by trying to differentiate env vars from regular characters—if an env var can't be inserted, the string should remain the same. - The reasons for having a vendor folder aren't clear to me if they haven't been patched: Why not just add Pipfile as a requirement? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1769 (comment)>, or mute the thread <https:/notifications/unsubscribe-auth/ABhjq1g38ED2ws8L-EwunnUpaS4Mc66Yks5tfI06gaJpZM4SuhjW> .

[dvf]

@techalchemy, I'm confused by your concern about package hashes. Maybe I'm missing something, but it would be weird if package hashes depended on the Pipfile itself.

Generally, not sure where it would ever make sense to have $ as a name.

It's bad to make assumptions about values: $ is an allowable character for an URL.

[jtratner]

`$` is an allowable character

Good point! Hmm...

…

On Fri, Mar 16, 2018 at 11:46 PM Daniel van Flymen ***@***.***> wrote: @techalchemy <https:/techalchemy>, I'm confused by your concern about package hashes. Maybe I'm missing something, but it would be weird if package hashes depended on the Pipfile itself. Generally, not sure where it would ever make sense to have $ as a name. It's bad to make assumptions about values: $ is an allowable character for an URL. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1769 (comment)>, or mute the thread <https:/notifications/unsubscribe-auth/ABhjq7JH49jWrD7wIdBSw6Vu8NFWX-z5ks5tfLFJgaJpZM4SuhjW> .

[dvf]

Thanks for everyone's interest.
I'm taking the contributors' advice and opening pypa/pipfile#105 instead.

[dvf]

@kennethreitz @jtratner @techalchemy do you guys mind taking a peek at pypa/pipfile#105

[techalchemy]

We vendor things to isolate them from system dependencies. This is a common practice.

[dvf]

I'm re-opening after taking suggestions and making the mirrored Pipfile pypa/pipfile#105

[kennethreitz]

🍰

[kennethreitz]

Made some changes to this — primarily, env vars are not propogated to the pipfile.lock, as they are here. They are also expanded at runtime. We want to keep secrets out of source control.