PEEP-002: Specify options via environment variables

[fridex]

No description provided.

[fridex]

The relevant implementation changes originally proposed in #2537

[kennethreitz]

'pipenv' should always be capitalized

[fridex]

Adjusted.

[kennethreitz]

I like this idea, as pip does this, but I think it would be difficult to implement with Click. @techalchemy thoughts?

[kennethreitz]

@fridex I meant like title case, Pipenv :)

[fridex]

@fridex I meant like title case, Pipenv :)

Fixed :)

I like this idea, as pip does this, but I think it would be difficult to implement with Click. @techalchemy thoughts?

Please check #2537 where this was implemented and discussed with @uranusjr.

[uranusjr]

As implied in in #2537, I am generally +1 with this proposal. There are, however, some (edge) cases that need to be addressed and clarified in the PEEP.

First of all, what format should the environment variables have? Should they always be PIPENV_<command>_<option>? A lot of commands in Pipenv have the same arguments (e.g. VERBOSE), and it could result in very awkward usage if we require PIPENV_INSTALL_VERBOSE, PIPENV_SYNC_VERBOSE, PIPENV_LOCK_VERBOSE all be set separately. Or, could we refactor some of those out to “command level”, and allow PIPENV_VERBOSE to work for all --verbose flags instead? How does Click handle this? (#2814 might be related to this as well.)

Second, what would this affect manually evaluated environment variables we currently have (in environments.py)? This has been partially mentioned in #2537, but I feel the PEEP should include a comprehensive section to explicitly explain how each existing variables should be handled. I would expect there are some nuances.

Thanks for the excellent (and the very first!) PEEP entry. I hope this is a good sign we will have a much better process in Pipenv going forward :D

[kennethreitz]

Agreed! I'll let you take it from here, @uranusjr. +1 from me, assuming all details get worked out.

[uranusjr]

(For @fridex: Feel free to update the PEEP and provide extra information regarding the topics I mentioned, including submitting additional PRs)

[techalchemy]

+1 from me as well

[fridex]

For @fridex: Feel free to update the PEEP and provide extra information regarding the topics I mentioned, including submitting additional PRs

Sure will do. Unfortunately I will not be available by the end of week and the week after, so please bear with my time. Thanks!

[sirosen]

I think we can do a better job of documenting this in click, but the auto_envvar_prefix code is designed to handle this type of case: https://github.com/pallets/click/blob/123dd717439d8620d8d6be5574d2c9f007952326/click/core.py#L318-L329

As long as you pass a prefix to the top level click context, it should percolate down to any options which have allow_from_autoenv=True.

You could therefore set the prefix to PIPENV and get reasonable behavior out of click, in which options to pipenv install would pull from PIPENV_INSTALL_* env vars.

Universal options like --verbose are probably most consistently handled by setting them to PIPENV_VERBOSE if you go down this path.

---

Aside: I think there's a very minor bug in the linked code, and I'll submit a fix which we should hopefully be able to get into click 7.

[fridex]

Aside: I think there's a very minor bug in the linked code, and I'll submit a fix which we should hopefully be able to get into click 7.

@sirosen, could you be more specific with the issue (if there needs to be done patching on Pipenv side to get changes in based on click 7 fixes)? Thanks!

[sirosen]

No problem.
The fix did make it in for click 7, so if pipenv is vendoring click without patching it, an upgrade should suffice.

If an older click is vendored or there's a patch you need to worry about conflicting, here's the exact change:
pallets/click#1105

If you passed an auto_envvar_prefix which was lowercase, click was not uppercasing it when it was passed explicitly. The uppercasing happened correctly when the envvar prefix was deduced from a parent command name.

[uranusjr]

I have a new question. Would it be possible to blacklist certain options, or whole options of commands from being accessible from envvar? There isn’t a use case for this right now, but I feel it would be best if we can have an idea what we can do to amend edge cases when they come up.

I don’t think we are patching our vendored Click. There are a few dependencies that have Click as a secondary dependency (e.g. click-completions), so we’ll need to make sure they can handle the Click 7.0 upgrade.

[uranusjr]

Also, before I forget, discussions in this thread will need to be summarised and written up in the PEEP document. It is not enough to add a link pointing to the issue page, since comments on GitHub can be edited, or even deleted.