-
-
Notifications
You must be signed in to change notification settings - Fork 724
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
Support environment variables in PipInstall
plugin
#8357
Support environment variables in PipInstall
plugin
#8357
Conversation
I don't really have an idea how to test this since we offload the functionality. When trying this manually on Coiled, it works. |
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 27 files ± 0 27 suites ±0 12h 6m 45s ⏱️ ±0s For more details on these failures, see this check. Results for commit dd6dc63. ± Comparison against base commit 412a0a9. This pull request removes 44 and adds 42 tests. Note that renamed tests count towards both.
This pull request removes 7 skipped tests and adds 2 skipped tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
50bd37c
to
b634646
Compare
python, *args, requirements_file = Popen.call_args[0][0] | ||
assert "python" in python | ||
assert args == ["-m", "pip", "install", "--upgrade", "-r"] |
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.
I get that it's difficult to test but I would add a comment here why the -r
is important. From first principle, this test is otherwise pretty useless. We test that pip install something but we also care that it is using the requirements file mechanism since this comes with additional features.
Adding a comment elevates this from a too detailed unit test that asserts on an implementation detail to an actual intended 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.
Fair point!
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.
Done
I've also added an example using an environment variable to the docs. |
Adds support for environment variables to the
PipInstall
plugin by using a requirement file under the hood (https://pip.pypa.io/en/stable/reference/requirements-file-format/#using-environment-variables). We should document this plugin better, but I would like to leave this to a dedicated PR.Blocked by (and includes) #8343
pre-commit run --all-files