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

Add an integration test with ansible-lint #250

Merged
merged 26 commits into from
Aug 9, 2024

Conversation

oraNod
Copy link
Contributor

@oraNod oraNod commented Jul 11, 2024

Resolves #170 by adding an integration test that checks compatibility with ansible-lint.

@oraNod oraNod requested a review from a team as a code owner July 11, 2024 09:57
@oraNod oraNod requested review from ssbarnea and Qalthos and removed request for a team July 11, 2024 09:57
@shatakshiiii shatakshiiii added the enhancement New feature or request label Jul 11, 2024
@oraNod
Copy link
Contributor Author

oraNod commented Jul 12, 2024

@shatakshiiii Hi, I've been wrangling with the assert in this test a bit more. I think we want to check that stdout for lint does not contain failures such as this one:

Failed: 2 failure(s), 0 warning(s) on 59 files. Last profile that met the validation criteria was 'min'.

Basically it needs to be something like assert ( "Failed: (\d) failure\(s\), (\d) warning\(s\) on (\d) files(.*)" in result.stdout is None) to ensure there are no lint failures.

I'm struggling to see what is in stdout when I run the test with tox. Is there an easy way to debug these tests? I feel like I'm doing a lot of guesswork and taking shots in the dark.

Thanks a mill for the help.

@oraNod
Copy link
Contributor Author

oraNod commented Jul 12, 2024

Also, why does it seem like vscode settings from this repo automatically format things in a way that is inconsistent with what I see in the auto fix commits? 6878fea

That's honestly driving me nuts. Every time I save in vscode formatting gets applied that conflicts with the formatting that gets applied with the pre-commit hooks. I don't think it's my local settings because it doesn't seem to happen with other repos.

@oraNod oraNod force-pushed the issue-170/lint-integration-test branch from 6878fea to b25730a Compare July 12, 2024 15:37
@oraNod
Copy link
Contributor Author

oraNod commented Jul 12, 2024

OK, I made a bunch more changes and figured out how to see what's going on with stdout and stderr. I know we'll probably want to remove some of the bits from the test before we merge but I thought I'd leave everything in so people can see what I'm doing.

Looks like this issue is coming from lint:

Standard Error: CRITICAL:root:Unable to create local directories(/home/ansible/.ansible/tmp): [Errno 13] Permission denied: b'/home/ansible'

I've been banging my head against this for the past while. Looking at this:

E       AssertionError: assert 3 == 0
E        +  where 3 = CalledProcessError(3, '/home/dnaro/git/ansible-creator/.tox/py/bin/ansible-lint /tmp/pytest-of-dnaro/pytest-22/popen-gw2/test_lint_collection0/collections/ansible_collections/testorg/testcol').returncod

That looks like I would expect it to. In fact if I try to reproduce this with my user installs of ansible-creator and ansible-lint I can't reproduce the error. I can scaffold a collection under /tmp/pytest-of-dnaro/pytest-22/popen-gw2/test_lint_collection0/collections/ansible_collections/ and then run ansible-lint against that directory and everything is fine.

Why is the cli run function trying to create a directory under /home/ansible?

@oraNod oraNod force-pushed the issue-170/lint-integration-test branch from 1c6ea54 to 0ac25aa Compare July 16, 2024 17:22
@NilashishC
Copy link
Collaborator

NilashishC commented Jul 17, 2024

Why is the cli run function trying to create a directory under /home/ansible?

So, the os.environ is set to /home/ansible in conftest (https://github.com/ansible/ansible-creator/blob/main/tests/conftest.py#L22). IIRC, this was done for some basic scaffolding tests.

Playing around a bit with test_lint.py, I think it's ansible-lint that's doing something with the home dir? One solution can be to change the hardcoded /home/ansible/ in conftest to use something like from pathlib import Path; str(Path.home()) and update all the affected fixtures.

@oraNod
Copy link
Contributor Author

oraNod commented Jul 17, 2024

Why is the cli run function trying to create a directory under /home/ansible?

So, the os.environ is set to /home/ansible in conftest (https://github.com/ansible/ansible-creator/blob/main/tests/conftest.py#L22). IIRC, this was done for some basic scaffolding tests.

Playing around a bit with test_lint.py, I think it's ansible-lint that's doing something with the home dir? One solution can be to change the hardcoded /home/ansible/ in conftest to use something like from pathlib import Path; str(Path.home()) and update all the affected fixtures.

Thanks for taking a look @NilashishC I can send a separate PR to adjust things in conftest.py.

@oraNod oraNod force-pushed the issue-170/lint-integration-test branch 2 times, most recently from 695e8ac to 7a37300 Compare July 24, 2024 13:56
@oraNod oraNod force-pushed the issue-170/lint-integration-test branch from 651a59a to e9da19e Compare July 24, 2024 19:34
@cidrblock
Copy link
Collaborator

Can you uses this for the ANSI function? It might get rid of the sonarcloud thing too. https://github.com/ansible/ansible-navigator/blob/88a240be6eeada2f579059210059fc3ecafc3037/src/ansible_navigator/utils/functions.py#L368

@oraNod
Copy link
Contributor Author

oraNod commented Jul 25, 2024

Can you uses this for the ANSI function? It might get rid of the sonarcloud thing too. https://github.com/ansible/ansible-navigator/blob/88a240be6eeada2f579059210059fc3ecafc3037/src/ansible_navigator/utils/functions.py#L368

@cidrblock Thanks, man. That's super helpful. I wasn't feeling great about that ANSI function in my commit. Hopefully I'll be able to take another look at this soon and keep hacking. Cheers.

@oraNod oraNod force-pushed the issue-170/lint-integration-test branch from 3a74e30 to 309ef44 Compare August 2, 2024 11:48
@oraNod oraNod requested a review from webknjaz August 2, 2024 15:52
@oraNod
Copy link
Contributor Author

oraNod commented Aug 2, 2024

I decided to add the noqa comments for ruff S603 after reading astral-sh/ruff#4054 I think the commands are constructed well enough and don't pose a risk from some kind of malicious injection. I kept banging my head against it and the only way to satisfy things might be to sanitize the commands by adding more complexity. That just seems over the top to me though but I'm open to advice from the experts here. Cheers.

@shatakshiiii shatakshiiii force-pushed the issue-170/lint-integration-test branch from 22cb454 to 7336089 Compare August 5, 2024 05:13
oraNod and others added 6 commits August 9, 2024 12:35
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
@oraNod oraNod force-pushed the issue-170/lint-integration-test branch from 7336089 to 1af83bb Compare August 9, 2024 12:33
@oraNod oraNod force-pushed the issue-170/lint-integration-test branch from a6609c7 to a473fe7 Compare August 9, 2024 12:39
pyproject.toml Outdated Show resolved Hide resolved
Copy link

sonarqubecloud bot commented Aug 9, 2024

@cidrblock cidrblock merged commit 6a94a15 into ansible:main Aug 9, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add ansible-lint integration test to check ansible-lint compatibility in the scaffolded content
5 participants