Run in an un-activated virtual environment, fixes #1507 #1866
Conversation
Note that this patch is designed to operate both before and after the fix for issue ansible#1507. The assertations similar to the pre-patched version must be the ones which fail against the pre-patched code. If these assertations pass but others fail then the problem is likely that the fix for issue ansible#1507 has not been applied.
kpinc
requested review from
relrod,
ganeshrn and
cidrblock
and removed request for
a team
| except ValueError: | ||
| return "" | ||
| del search_path[pos] | ||
| monkeypatch.setenv("PATH", ";".join(search_path)) |
There was a problem hiding this comment.
I believe this meant to be
| monkeypatch.setenv("PATH", ";".join(search_path)) | |
| monkeypatch.setenv("PATH", os.path.sep.join(search_path)) |
*nix separator is : normally while windows uses ;
There was a problem hiding this comment.
@webknjaz And you know very well that ansible-lint does not run on Windows.
There was a problem hiding this comment.
I know but it's still good practice not to hardcode things regardless.
There was a problem hiding this comment.
And your suggestion is misleading as os.path.sep is / while os.pathsep is :.-- not sure if just using the : not not just be much easier to read.
A simple dot... totally different outcome.
|
|
||
|
|
||
| @pytest.fixture | ||
| def deactivate_venv(monkeypatch: MonkeyPatch) -> Deactivator: |
There was a problem hiding this comment.
I like the use of monkeypatch but returning a callable seems like an overengineering: since all the tests call it at the beginning of their execution, this fixture could do this when injected.
Assuming you're worried/don't know how this works with parametrize — it's quite simple: just inject a request fixture here and access the parameter via request.param. And on the test function side, add indirect=('deactivate_venv', ) argument into the parametrize() decorator. That's it!
| """Deliver a function to deactivate the current venv, if any, if requested, by removing it's bin dir from $PATH.""" | ||
|
|
||
| def deactiveator(deactivate: bool) -> str: | ||
| if deactivate: |
There was a problem hiding this comment.
Pro tip: this clause can be turned into a "guard expression" by flipping the check. Use if not request.param: return "" to exit early and this will allow dedenting a whole bunch of code making the control flow look more linear.
| # it demonstrates that ansible-lint does not run in a virtual environment | ||
| # unless that environment is activated. | ||
| assert re.search( | ||
| "Running [^ ]*ansible-galaxy role install", result.stderr |
There was a problem hiding this comment.
Why not do a more precise comparison with
| "Running [^ ]*ansible-galaxy role install", result.stderr | |
| f"Running {execdir}/ansible-galaxy role install", result.stderr |
?
|
|
||
|
|
||
| def run_ansible_version() -> CompletedProcess: | ||
| """Run `ansible --version` and return the result.""" |
There was a problem hiding this comment.
This will fix the failing docs build:
| """Run `ansible --version` and return the result.""" | |
| """Run ``ansible --version`` and return the result.""" |
|
On Fri, 11 Feb 2022 00:01:16 -0800 Sorin Sbarnea ***@***.***> wrote:
@kpinc Why do you think that approach is better than the one from
#1860 ?
I believe I like your approach better.
I am bit surprised about the number of lines of code involved in as I
was able to achieve (the same?) using far less. Please take a look at
my PR and see if you can spot some fatal flaws.
I have comments and see a couple of improvements to be made.
I don't know how the workflow should be handled. My plan is
to submit a PR to your repo, including my test case with changes based
on the other feedback I've gotten, and then let you
incorporate changes into your PR, #8160. Is there a better way?
Regards,
Karl ***@***.***>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
|
Fix so the command line ansible-lint will run in an un-activated virtual environment, via a pathname supplied to the shell. Fixes issue #1507.
THEORY:
The foundational observation is that because activating a virtual environment does only one thing, add a path to the venv's $PATH, the only thing we know about an un-activated venv is that the venv[0] of it's process is either not in a $PATH directory or, if it is, then the venv is, in effect, activated. Therefore the only way to possibly test for virtual environment activation is to check venv[0] against $PATH.
The underlying premise is that if it is necessary to specify a path, one that contains directories, to execute the python which runs ansible-lint, then the other ansible-* commands run by ansible-lint reside in the os.path.basedir() of that directory. Alternatively, given that the operational test to determine if ansible-lint is running in an un-activated virtual environment is whether a path containing a directory designation is required to execute the running ansible-lint, then, having decided that this is the case, use the directory designation to execute the other ansible-* commands of the ansible-lint's virtual environment.
The approach taken here is a heuristic, one which I believe is explicit and clear. The alternative, as far as I've been able to imagine, is to fallback to trying to execute in an un-activated venv as a fallback after the search through $PATH fails. (Or vice-versa.) This also is a heuristic. Comparison without code is uninteresting, but it seems to me that the corner cases that fail using either heuristic would involve the case of someone running some ansible-* commands in some system/virtual environments and some in another. It might make sense to simply declare this a configuration that is not supported.
PRACTICE:
Obtains directory portion of sys.executable and checks if it is an element of $PATH. If so the system can rely on $PATH to discover all executables; the original codepath is used and basenames are the argv[0] given to subprocess.run(). If not then the Python binary was run with a non-trivial pathname, as when executing a binary in an un-activated virtual environment's bin directory; the directory portion of sys.executable is prefaced to argv[[0] when executing ansible-related commands.
Testing is done by monkeypatching to remove the element of $PATH which tox adds to activate the virtual environment. Existing tests which run the ansible-lint executable were modified to run twice, once as-is and a 2nd time with the virtual environment deactivated.
Further, to directly test what a user would do, one test was also modified to, in addition to existing codepaths, run the ansible-lint executable installed in the tox virtual environment. The existing test runs via 'python -m ansiblelint'.