Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add support for "packages" install key in v2 config #7289
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
Add support for "packages" install key in v2 config #7289
Changes from 8 commits
9d018b0
7e57736
4582e58
2f9c98b
899667e
bf773e8
d906366
c776387
3362072
0a99cbb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider supporting private packages https://docs.readthedocs.io/en/stable/guides/private-python-packages.html for Read the Docs for Business, probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I am missing something, this should already be supported implicitly. You can just use a Git repository URL instead of a package name, e.g.:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested that the environment variable is properly replaced with its value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope and I doubt that it will be at this stage. Good point that we should probably support the substitution of environment variables, otherwise while it technically works to hard code the token, that's probably not what anyone wants 😄
Does Pip support the substitution in
requirements.txt
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, pip supports expanding variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it'll be easier to just write the arguments into a temporary requirements file and re-use the existing logic for installing from that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to push this along and finish this work, I'd be fine opening an issue to add private repository support to the
packages
config key later. This PR seems close otherwise and it looks to be blocked on this additional feature.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be careful here, users could set a package name like
--option
and we will execute that. Not sure the best way to prevent that. We could probably validate this in the config module and raise an exception for invalid names, or maybe escape those characters here. /cc @readthedocs/coreInvalid names would be those that start with
-
, not sure what else we should validate here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much of a concern is this really? Pip also reads options from
requirements.txt
files, or at least a subset of options that are valid for thepip install
command likeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, you are right, maybe allow the same subset of options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about using the same parser that pip uses and check for a valid name?
On the other hand, @davidfischer created this package that could be useful if we can validate each line separately: https://requirements-parser.readthedocs.io/en/latest/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does RTD have a separate validation step? Otherwise Pip will just complain if the requirement is invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but we need to verify it before sending to pip in some way because otherwise you can inject random commands into the
pip
by callingpip install package1 "echo test > /tmp/test.txt"
(quotes should be `, but I can escape them here)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This package is super old and outdated and probably would need some love before it's useful. That package doesn't directly parse the lines by itself but instead uses the parser built-in to setuptools.