Add expansion of environment variables in requirement files

[BrownTruck]

I really like pip and have been using it for quite some time. One scenario that I've come across several times that doesn't seem to be covered is installing from private git (or other) repos without having to store private API keys or other authentication artifacts in the requirements file.

In a 12-factor fashion, I've started thinking about how pip could support private repos by keeping secrets in environment variables. And it turned out that supporting the expansion of environment variables is pretty trivial. Adding a line like this in a requirement file:

https://$GITHUB_TOKEN:[email protected]/user/repo/archive/master.zip

can easily be expanded to by extracting the actual API key from the $GITHUB_TOKEN variable using os.path.expandvars, turning it into:

https://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx:[email protected]/user/repo/archive/master.zip

I'm not sure if there are any caveats with this implementation around cross-platform support, security or whatever else. But I think this would be a great feature to add to pip and would love to know how to improve on this PR to get it added.

I'm also aware that the tests could be improved since they are not really testing if this actually work. However, mocking the environment for this specific test is not really straight forward. I'd appreciate any input on how this can be tested properly.

---

This was automatically migrated from #3514 to reparent it to the master branch. Please see original pull request for any previous discussion.

Original Submitter: @elbaschid

---

This change is 

[wobeng]

Has this been implemented?

[chaitan94]

While the test is still reasonably accurate, isn't this supposed to be https://${WRONG_FORMAT}:$NON_EXISTING_VARIABLE@ instead?

[pradyunsg]

🎨 NON_EXISTENT_VARIABLE

[chaitan94]

Would love to see this feature merged. May I ask if there are any plans of doing this anytime soon?

[silverzhaojr]

This is a wonderful feature! Any update when it will be merged?

[pradyunsg]

The change looks good to me in general.

That said, before this can be merged, this PR would need to be rebased or merged with the current master and a news fragment needs to be added for this work.

[RonnyPfannschmidt]

also this seems to be a less complete implementation of https://docs.python.org/2/library/os.path.html#os.path.expandvars

[jorng]

@RonnyPfannschmidt yeah, I'm not sure I understand the reasoning for that, but it was done in response to a comment from @pfmoore last year:

I hadn't spotted the expandvars usage. For this PR to be acceptable, I think that the variable expansion used has to be platform independent (otherwise requirements files become platform dependent which is not acceptable). So the patch needs to do its own expansion, and document the format used.

Isn't os.path.expandvars already platform independent? According to the docs, the $name or ${name} style work on all platforms. Making custom expansion code seems like a poor choice to me.

[benoit-pierre]

@jediorange: Yes, but os.path.expandvars also does %var% expansion on Windows, and only on that platform.

[jorng]

@benoit-pierre I see that, but just personally fail to understand how that is a bad thing. The use case for environment variables in requirements files is for using private repos or obscuring other things. This means that they are used for private or proprietary projects, not mainstream things that are going to be downloaded by everyone.

If someone really wants to use Windows style variables for a personal/proprietary project (breaking things for any other OS), why not let them? I would consider it expected behavior.

[pradyunsg]

this seems to be a less complete implementation of [link to os.path.expandvars docs]

As @jediorange and @benoit-pierre pointed out, the history on this is that the original PR was implemented using os.path.expandvars but in favour of not having too many ways and os-dependent behaviour to specify variable-expansion (like expandvars provides) it was decided to have just one way to it, using the POSIX standard definition. That way, there's consistency across platforms.

Making custom expansion code seems like a poor choice to me.

Above + this custom logic is straightforward enough.

[pradyunsg]

why not let them?

If we can prevent someone from breaking stuff by mistake (or ignorance) without handicapping their ability to do something (here - expand variables in req-files), I'd say that we should. :)

[jorng]

If we can prevent someone from breaking stuff by mistake (or ignorance) without handicapping their ability to do something (here - expand variables in req-files), I'd say that we should. :)

@pradyunsg I would argue that more people would be confused by having to adhere to a very specific style of environment variables, rather than using the same styles they can anywhere else (in shell scripts, Procfiles, etc).

To be clear: While I don't see the purpose of making it limited to only ${}, I am in favor of getting this feature added either way.

[pradyunsg]

@jediorange

I would argue that more people would be confused

I mean, yeah, it's possible. The nice thing about being strict initially is we could expand this behaviour (word-play not intended) later if it actually confuses/annoys too much. :)

While I don't see the purpose of making it limited to only ${}, I am in favor of getting this feature added either way.

Roger that!

[pfmoore]

One outstanding issue from the discussion in the original PR is that this needs documentation - both to note that the requirements file format allows variable expansion, and to describe the permissible form.

The key review comment from the previous discussion was #3514 (comment). Other than that @xavfernandez was lukewarm on the idea, as am I. None of the other @pypa/pip-committers has commented yet.

If @elbaschid wants to take this PR forward by adding docs and fixing up the requested changes, or if someone else wants to take it over, then I'm OK to merge this. But I'd like someone with an actual need for the feature to complete the work.

[avinashak]

I recently figured out a way to achieve this without changes to pip utilizing git's url rewrite config

So doing something like git config --global "url.https://${GITHUB_TOKEN}@github.aaakk.us.kg.insteadOf" https://github.com will let you install packages from your private repo's if you have the ability to configure git in your environment.

Hope this helps.

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

[pradyunsg]

this needs documentation - both to note that the requirements file format allows variable expansion, and to describe the permissible form.

If @elbaschid or anyone else wants to take this forward, this is one thing that needs to be fixed.

[roadsideseb]

@pradyunsg I've rebased and added the NEWS file as requested. Seems like the failed test are the same as in master. I'll add documentation and make sure tests are running as soon as they are fixed on master.

I'm thinking about adding a section in docs/reference/pip_install.rst below "VCS Support". Does that sound like a reasonable place?

[pradyunsg]

VCS support is probably not the right place. You want to do it at the location where there's discussion on/description of the requirement file format. I think that would in pip install reference but yeah, whereever it is, I think that's the right place.

[pradyunsg]

LGTM.

I'll wait for another person from @pypa/pip-committers to review. :)

[roadsideseb]

I'm happy to make the 🎨 changes...I fully understand your picky-ness 😃 .

I've also updated the docs according to your suggestions. I hope that makes more sense now and is concise enough. Let me know if you need any other changes made.

[roadsideseb]

@pradyunsg I realized the test on appveyor were still failing so I rebased the latest working master and that fixed it. All seems to be green on that front now. Let me know if you need me to make any changes before this can be merged.

[venthur]

Thanks for working on this PR! This feature is badly needed in corporate environments where you have to install dependencies from private repositories with auth. This Issue is probably related to this PR.

[n1k0]

What's the status of this PR? Our company would love seeing this available in pip as it would solve tons of headaches.

[nacivida]

We also need this feature for security. Any change this could be merged soon?

[eacmen]

I concur. We use travis-ci and this feature would make our process much cleaner and more secure with our private pypi repo.

[pfmoore]

I remain lukewarm on the functionality, but I'm OK for this to be merged.

[nacivida]

Thanks for approvals, we would be grateful if a collaborator merges this in. Thanks.

[pradyunsg]

Given that none of the @pypa/pip-committers are opposed (or in favour) of this and, that there's multiple people asking for this and the PR is complete, I'm okay with merging this.

I should state that this increases the complexity of the format that is already defined on an as-implemented basis, which I don't like but again, I won't oppose this for that reason.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.