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 use without activation of virtualenv #1860

Merged
merged 1 commit into from
Feb 11, 2022
Merged

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Feb 10, 2022

Allow users to call ansible-lint with a full path even when it is installed inside an inactive virtualenv.

This change also includes a fail-safe test that prevents future regressions.

Fixes: #1507
Closes: #1866 (duplicate)

@ssbarnea ssbarnea added the bug label Feb 10, 2022
@ssbarnea ssbarnea force-pushed the fix/1507 branch 4 times, most recently from 2f6dc7e to 7340dd4 Compare February 10, 2022 16:29
ssbarnea added a commit to ssbarnea/ansible-lint that referenced this pull request Feb 10, 2022
ssbarnea added a commit to ssbarnea/ansible-lint that referenced this pull request Feb 10, 2022
ssbarnea added a commit that referenced this pull request Feb 10, 2022
@ssbarnea ssbarnea marked this pull request as ready for review February 10, 2022 17:26
@ssbarnea ssbarnea requested a review from a team as a code owner February 10, 2022 17:26
@ssbarnea ssbarnea requested review from relrod and priyamsahoo and removed request for a team February 10, 2022 17:26
src/ansiblelint/_preimport.py Outdated Show resolved Hide resolved
src/ansiblelint/_preimport.py Outdated Show resolved Hide resolved
src/ansiblelint/__main__.py Outdated Show resolved Hide resolved
@ssbarnea ssbarnea force-pushed the fix/1507 branch 2 times, most recently from 939feea to 3eba0a3 Compare February 11, 2022 14:15
src/ansiblelint/config.py Outdated Show resolved Hide resolved
src/ansiblelint/config.py Outdated Show resolved Hide resolved
test/test_config.py Outdated Show resolved Hide resolved
src/ansiblelint/config.py Outdated Show resolved Hide resolved
@ssbarnea ssbarnea force-pushed the fix/1507 branch 2 times, most recently from c7411b6 to 7bd4ace Compare February 11, 2022 14:55
test/test_config.py Outdated Show resolved Hide resolved
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

I just saw #1866 and I like it better because of a more accurate approach and no import-time patches.

test/test_config.py Outdated Show resolved Hide resolved
@ssbarnea
Copy link
Member Author

@webknjaz I had a talk with @cidrblock and I will attempt to remove the code running at import from linter before doing this change. If it works we may be able to ditch that anti-pattern.

@ssbarnea
Copy link
Member Author

I am very aware about the other change which is altering a huge amount of code, likely 10x more than this one and that is a big deal.

I do not want to modify all subprocess calls to inject full paths in order to address this issue. That may come back to us very soon and it might conflict with other use cases and we could easily introduce bugs when calling other processes and forgetting to add the magic sanitization call to them. At least the PATH adulteration at start is clear and ensures that whatever we call will be using the same rules.

I also sorted the import issues and avoided run of code as import.

@ssbarnea ssbarnea changed the title Allow use without pre-activation of virtualenv Allow use without activation of virtualenv Feb 11, 2022
@ssbarnea ssbarnea merged commit 34a2798 into ansible:main Feb 11, 2022
@ssbarnea ssbarnea deleted the fix/1507 branch February 11, 2022 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ansible-lint does not look for ansible binary inside venv
4 participants