Skip to content
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.

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

Allow adding a custom string to pip's User-Agent via an environment variable #5550

Merged
merged 12 commits into from
Mar 31, 2019

Conversation

conspicuousClockwork
Copy link
Contributor

Addresses issue #5549.

@conspicuousClockwork conspicuousClockwork changed the title #5549 Add provisional string to User-Agent with `PIP_USER_AGENT_PROVISIONAL_STRING Add provisional string to User-Agent with `PIP_USER_AGENT_PROVISIONAL_STRING Jun 28, 2018
@conspicuousClockwork conspicuousClockwork changed the title Add provisional string to User-Agent with `PIP_USER_AGENT_PROVISIONAL_STRING Add provisional string to User-Agent with PIP_USER_AGENT_PROVISIONAL_STRING Jun 28, 2018
Copy link
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

Out of curiosity, why did you choose the word "provisional"? To me, something like "custom" or "user-data" or "user-provided" would convey the meaning more clearly.

tests/unit/test_download.py Outdated Show resolved Hide resolved
tests/unit/test_download.py Outdated Show resolved Hide resolved
src/pip/_internal/download.py Outdated Show resolved Hide resolved
@dstufft
Copy link
Member

dstufft commented Feb 17, 2019

FWIW this will break PyPI's ability to parse these user agents, which might be OK.

@cjerdonek
Copy link
Member

You mean PyPI breaks on new / unknown keys in the JSON? Is that intentional?

@dstufft
Copy link
Member

dstufft commented Feb 17, 2019

Eh, I misread the PR because I had just woken up. I thought it was just appending data to the end of the UA, adding rando keys to the User Agent should be fine.

@conspicuousClockwork
Copy link
Contributor Author

Out of curiosity, why did you choose the word "provisional"? To me, something like "custom" or "user-data" or "user-provided" would convey the meaning more clearly.

Back when I made the PR there were a few points made about not replacing the entire user-agent header, so the intent was to provide a way to add text in provision to the original data in the user-agent header. In any case, I don't believe there's any particular reason to keep the word "provisional". I'll update the PR with any preferred verbiage @cjerdonek.

@BrownTruck
Copy link
Contributor

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 be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 25, 2019
@pradyunsg
Copy link
Member

I like "user-data". If we're adding this, it should get documented.

Idk if it'll make it into the BigQuery dB as is but it'd be best to include that in the documentation too.

@conspicuousClockwork
Copy link
Contributor Author

@pradyunsg PIP_USER_AGENT_USER_DATA? And as far as documentation goes, is there anywhere else that this needs to be documented besides news?

@cjerdonek
Copy link
Member

cjerdonek commented Feb 26, 2019

I like user-data and PIP_USER_AGENT_USER_DATA, too.

For documentation, it doesn't seem like there's one section in the docs listing all the environment variables that are supported (aside from environment variables that correspond to command-line options). Since this is a pretty specialized variable and was related to your proxy server use case, maybe it can be added as a sentence at the end of this short section:
https://pip.pypa.io/en/stable/user_guide/#using-a-proxy-server

Alternatively, maybe a section called "General Environment Variables" could be added after "General Options" here: https://pip.pypa.io/en/stable/reference/pip/#general-options
That could list environment variables that apply to pip's operation as a whole that don't have a corresponding command-line option. (This variable doesn't seem worth also adding as a command-line option, since that adds extra maintenance overhead.)

@conspicuousClockwork
Copy link
Contributor Author

@cjerdonek @pradyunsg is the additional documentation something that can be added to this PR or is that a separate issue?

@cjerdonek
Copy link
Member

The same. Documentation of a change should go in the same PR as the change itself. It probably only needs a sentence or two.

@cjerdonek cjerdonek changed the title Add provisional string to User-Agent with PIP_USER_AGENT_PROVISIONAL_STRING Allow adding a custom string to pip's User-Agent via an environment variable Mar 1, 2019
Copy link
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

A couple comments in advance of the docs change.

news/5549.feature Outdated Show resolved Hide resolved
src/pip/_internal/download.py Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
@conspicuousClockwork
Copy link
Contributor Author

Am I missing something here or is the pipeline broken? I can't find anything in the build logs indicating a direct issue with my changes.

@cjerdonek
Copy link
Member

I opened a PR to fix it, which is why I wasn’t able to do anything here.

@cjerdonek
Copy link
Member

@conspicuousClockwork Can you rebase your changes now that CI has been fixed?

@cjerdonek cjerdonek added the type: enhancement Improvements to functionality label Mar 31, 2019
@cjerdonek cjerdonek merged commit ac9010e into pypa:master Mar 31, 2019
@cjerdonek
Copy link
Member

Thanks for your work on this, @conspicuousClockwork!

@lock
Copy link

lock bot commented May 28, 2019

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.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants