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.
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
|
I've also added an example using an environment variable to the docs. |
Adds support for environment variables to the
PipInstallplugin 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