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

fix: use ansible-compat to install collections #2466

Merged
merged 10 commits into from Sep 23, 2022
Merged

Conversation

mnaser
Copy link
Contributor

@mnaser mnaser commented Sep 20, 2022

This patch switches ansible-lint to start using ansible-compat to install a collection if needed, this removes some duplicated code and also solves issues where both molecule and ansible-lint and running on the same system and fighting against each other (one creates symlinks, the other copies content).

Fixes #2426

@mnaser mnaser requested review from a team as code owners September 20, 2022 13:36
@mnaser mnaser closed this Sep 20, 2022
@mnaser mnaser reopened this Sep 20, 2022
@github-actions github-actions bot added the bug label Sep 20, 2022
@mnaser
Copy link
Contributor Author

mnaser commented Sep 20, 2022

@ssbarnea can I add a commit to bump the schema here or?

@ssbarnea
Copy link
Member

I am fixing it separately before merging this fix but it is ok to include in your patch, just to enable testing.

@mnaser
Copy link
Contributor Author

mnaser commented Sep 20, 2022

@ssbarnea should we drop most of those tests since they're essentially "testing" ansible-compat ?

@heindsight
Copy link

Nice one @mnaser.

I can confirm this passes (a slightly adjusted version of) the test I wrote to reproduce the same bug. See

@ssbarnea ssbarnea added the incomplete Additional work or information is required label Sep 21, 2022
@ssbarnea
Copy link
Member

I want to get this in but we need to pass all tests.

These tests are no longer useful since they are checking for a behaviour
that is no longer present in the code, but handled by `ansible-compat`.
@mnaser
Copy link
Contributor Author

mnaser commented Sep 21, 2022

ok, the reason it's pretty borked right now is that ansible-compat will check if the parent folder has a galaxy.yml and if so, install that as a collection.

however, in our case, it's an example (but invalid one) so the tests are failing.

https://github.com/ansible/ansible-compat/blob/main/src/ansible_compat/runtime.py#L354-L359

@ssbarnea what do you suggest in this case? can we rename that file? move it into its own folder? the current examples layout seems to make ansible-lint feel that it's a collection but it's galaxy.yml has invalid dependencies.

@ssbarnea
Copy link
Member

ok, the reason it's pretty borked right now is that ansible-compat will check if the parent folder has a galaxy.yml and if so, install that as a collection.

however, in our case, it's an example (but invalid one) so the tests are failing.

https://github.com/ansible/ansible-compat/blob/main/src/ansible_compat/runtime.py#L354-L359

@ssbarnea what do you suggest in this case? can we rename that file? move it into its own folder? the current examples layout seems to make ansible-lint feel that it's a collection but it's galaxy.yml has invalid dependencies.

Hmm. Thanks point pointing this shady code. If the tests are failing because it happens to find a broken-example of a galaxy.yml file in a parent directory, just move the tests, or move the broken galaxy file (which is likely used by another test).

If you identify the fix, you can also opt-in to make a different PR, so it would be easier to review.

The example galaxy.yml is invalid since it points to non-existant
collections, this breaks some of the other tests which try and build
a runtime and try and install it.
@mnaser
Copy link
Contributor Author

mnaser commented Sep 22, 2022

@ssbarnea I'm not sure why some macOS tests are failing and why the eco jobs are failing

could you give me a hand if you have a few extra minutes?

@ssbarnea
Copy link
Member

@mnaser is not your fault. We already spotted it few hours ago. I suspect Github updated the runners and put some surprises for us. I seen that ansible is failing syntax check in that particular case but we were not able reproduce.

I have a change to increase pytest verbosity and hopefully to find the root cause of the failure.

In general if all linux jobs are passing, your change is good (WSL is also unstable)

@mnaser
Copy link
Contributor Author

mnaser commented Sep 22, 2022

@mnaser is not your fault. We already spotted it few hours ago. I suspect Github updated the runners and put some surprises for us. I seen that ansible is failing syntax check in that particular case but we were not able reproduce.

I have a change to increase pytest verbosity and hopefully to find the root cause of the failure.

In general if all linux jobs are passing, your change is good (WSL is also unstable)

cool, well in that case, the change is good to go

@ssbarnea ssbarnea added incomplete Additional work or information is required and removed incomplete Additional work or information is required labels Sep 22, 2022
@ssbarnea
Copy link
Member

@mnaser Sorry but I spotted a regression affecting one of the eco repositories, look at this job and you will something like:

 ERROR! Unexpected Exception, this is probably a bug: Cannot call rmtree on a symbolic link

This is for sure related to this change. Now, it might be possible that the bug is inside ansible-compat, but still, merging this fix, would require us to fix that bug first.

You can run the same locally if you want, tox -e eco.

@ssbarnea
Copy link
Member

ssbarnea commented Sep 22, 2022

@mnaser I raised ansible/ansible-compat#165 but I am not really sure about the solution.

Basically we need to avoid trying to install current collection using ansible-galaxy if we already symlinked it, as the tool will fail. If we do not symlink it, we might not be able to obtain good error messages while linting, as they would be originating from outside the current repository.

I hope to see more contributions from you in the future. Maybe we can talk at some point about zuul-jobs, which seems to bit slow in fixing the issues reported by the linter. Maybe we can join forces with @ianw to make it full green.

@ssbarnea ssbarnea removed the incomplete Additional work or information is required label Sep 23, 2022
@ssbarnea
Copy link
Member

@mnaser I fixed compat, made a new release and updated this PR to make use of it. I also checked the other two broken jobs but the errors reported on them are unrelated to these changes so I will merge it.

@ssbarnea ssbarnea merged commit e248b64 into ansible:main Sep 23, 2022
@mnaser mnaser deleted the fix/2426 branch September 24, 2022 02:26
davedittrich added a commit to davedittrich/ansible-lint that referenced this pull request Sep 27, 2022
* fix: use ansible-compat to install collections

* chore: clean-up pylint failures

* test: drop mocking tests

These tests are no longer useful since they are checking for a behaviour
that is no longer present in the code, but handled by `ansible-compat`.

* fix: move example galaxy.yml file for tests

The example galaxy.yml is invalid since it points to non-existant
collections, this breaks some of the other tests which try and build
a runtime and try and install it.

* fix: drop PYTEST_REQPASS

* Bump compat

Co-authored-by: Sorin Sbarnea <ssbarnea@redhat.com>
Co-authored-by: Sorin Sbarnea <sorin.sbarnea@gmail.com>
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 fails when linting a collection installed within ansible-compat cache
3 participants