Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove code that modifies ansible import paths #4380

Merged
merged 1 commit into from
Jan 27, 2025
Merged

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Jan 27, 2025

As part of new test-isolation strategy, molecule will no longer take
care itself about modification of:

  • ANSIBLE_COLLECTIONS_PATH
  • ANSIBLE_ROLES_PATH
  • ANSIBLE_LIBRARY
  • ANSIBLE_FILTER_PLUGINS

Test isolation is supposed to be covered transparently by ansible-compat runtime when code runs inside a virtual environment. Molecule is already using it for running all its external commands.

Related: https://ansible.readthedocs.io/projects/dev-tools/user-guide/test-isolation/

@ssbarnea ssbarnea requested a review from a team as a code owner January 27, 2025 12:03
@ssbarnea ssbarnea added the major label Jan 27, 2025
@ssbarnea ssbarnea force-pushed the major/test-isolation branch 2 times, most recently from 9a7d114 to 19b6a10 Compare January 27, 2025 12:35
As part of new test-isolation strategy, molecule will no longer take
care itself about modification of:

- ANSIBLE_COLLECTIONS_PATH
- ANSIBLE_ROLES_PATH
- ANSIBLE_LIBRARY
- ANSIBLE_FILTER_PLUGINS

Test isolation is supposed to be covered transparently by
ansible-compat runtime when code runs inside a virtual environment.

Related: https://ansible.readthedocs.io/projects/dev-tools/user-guide/test-isolation/
@ssbarnea ssbarnea force-pushed the major/test-isolation branch from 19b6a10 to 68c96a4 Compare January 27, 2025 12:40
@ssbarnea ssbarnea merged commit 30a9749 into main Jan 27, 2025
21 of 23 checks passed
@ssbarnea ssbarnea deleted the major/test-isolation branch January 27, 2025 14:15
@jsf9k
Copy link
Contributor

jsf9k commented Jan 28, 2025

@ssbarnea - This breaks my molecule setup (and probably that of many other folks), and I'm not sure the best way to fix it. See, e.g., the most recent GitHub Actions runs from cisagov/ansible-role-assessment-tool#62 where molecule can no longer find the role that is being tested (ansible-role-assessment-tool). I'd like molecule to find and use the local version of the role that is in the code checked out from GitHub, since that is the version I want to be tested. In this case, am I now expected to set an environment variable before running molecule to add the local project directory to the role path, since molecule no longer does this for me?

In cisagov/skeleton-ansible-role#217 I tried explicitly listing the role to be tested in molecule.default/requirements.yml as a solution, but if I do that and modify the Ansible code for the role in a feature branch then running molecule from said feature branch will not pick up the local changes (since it will pull the default branch from the GitHub repo). This is obviously subpar.

If there are more changes forthcoming that will fix this then I can pin to <25.2.0 for now, but I'm not sure if that is the case.

@alix-rm
Copy link

alix-rm commented Jan 29, 2025

Hello, I concurr with the above comment by @jsf9k.

@ssbarnea Could you please explain the correct way to upgrade to version 25.2.0 of molecule for the common layout of ansible-role-myrole/molecule/default?

I understand the rationale of test isolation behind the change but my feeling is that testing a local role's code is the first and most common thing people do with molecule.

I think this change could deter most the maintainers of the most common OSS roles from upgrading to this version of molecule and thus following current best practices. For instance all the geerlinguy. roles use this layout.

First thing most people will be tempted to do is pin molecule to < 25.2.0, which is probably not healthy in the long run.

Happy do discuss this and available to help easing the migration.

jsf9k added a commit to cisagov/skeleton-ansible-role that referenced this pull request Jan 30, 2025
Molecule used to modify the roles path for us, but as of v25.2.0 no
longer does.  (See ansible/molecule#4380 for details.)  As a result we
must now modify it ourselves.
@remod
Copy link

remod commented Jan 30, 2025

We fixed it by explicitly setting ANSIBLE_COLLECTIONS_PATH=/path/to/collection before invoking molecule.

@jsf9k
Copy link
Contributor

jsf9k commented Jan 30, 2025

cisagov/skeleton-ansible-role#221 shows how I am intending to work around this. Very similar to @remod's solution.

@pierrehenrymuller
Copy link

Molecule and Ansible must behave in exactly the same way when searching for roles. Ansible has its own rules, and Molecule must test that Ansible will work properly in production. Changing this on the Molecule side no longer guarantees that when Ansible is running, everything will work properly.

Everything's broken with this new version, and a step backwards is imperative.

How does Molecule benefit from managing things differently?
What is the need to randomize functional testing?

Adding an environment variable to return to normal operation is not an efficient solution.

hswong3i added a commit to alvistack/ansible-community-molecule that referenced this pull request Feb 5, 2025
    git clean -xdf
    tar zcvf ../python-molecule_25.2.0.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp python-molecule.spec ../python-molecule_25.2.0-1.spec
    cp ../python*-molecule*25.2.0*.{gz,xz,spec,dsc} /osc/home\:alvistack/ansible-community-molecule-25.2.0/
    rm -rf ../python*-molecule*25.2.0*.*

See ansible#4380

Signed-off-by: Wong Hoi Sing Edison <hswong3i@pantarei-design.com>
@hswong3i
Copy link
Contributor

hswong3i commented Feb 5, 2025

Once I revert this PR my CI case working fine again:

@alix-rm
Copy link

alix-rm commented Feb 6, 2025

@ssbarnea would you care to comment on these changes?

From a user perspective, your commits through ansible-compat, molecule and even ansible-lint just broke a lot of people's workflows. The documentation is not sufficient for such drastic changes and unexpected for a minor version.

@dgibbs64
Copy link

dgibbs64 commented Feb 6, 2025

Can confirm testing of all my ansible roles is now broken since update to 25.2.0. Getting the following error. Is there a workaround or update planned to release asap?

ansible_compat.errors.AnsibleCommandError: Got 250 exit code while running: ansible-galaxy collection install -r /home/runner/work/ansible-role-nodejs/ansible-role-nodejs/requirements.yml

@matthijs-oosterhoff
Copy link

These changes seem to break setups using molecule-plugins[vagrant] (and I suppose more), see ansible-community/molecule-plugins#301.

Setting the ANSIBLE_LIBRARY variable to something like <your_venv>/lib/python<version>/site-packages/molecule_plugins/vagrant/modules resolves the issue. But I don't think it's really user friendly to have to set this variable before running molecule. Am I missing something? What is the advised way to manage these variables, now Molecule no longer takes care of them?

@joerg
Copy link

joerg commented Feb 7, 2025

For us this release also breaks all our roles and molecule tests... and I am not sure why. We have pretty simple and standard roles and molecule/ansible is set up via venv. I tried reading through all the linked documentation that I found and tested some approaches but none of them seem to make any difference.
I think it has to do with the test isolation linked in the original comment but if the role I am develping and testing is not automatically picked up by molecule then molecule is simply not doing its job. Probably I am just missing some smaller changes required on my side but these should be documented somewhere at least.

@chriscroome
Copy link

FWIW when running Molecule in GitLab CI you can solve this issue by adding this at the top of the .gitlab-ci.yml file:

variables:
    ANSIBLE_ROLES_PATH: "${CI_PROJECT_DIR}/..:~/.ansible/roles:/usr/share/ansible/roles:/etc/ansible/roles"

This might be worth adding to the docs.

chriscroome added a commit to webarch-coop/ansible-role-ansible that referenced this pull request Feb 10, 2025
@dgibbs64
Copy link

dgibbs64 commented Feb 10, 2025

@ssbarnea Are there plans to revert this change or provide any documentation on this? This change has broken a lot of peoples workflows with no warning or documentation on what we need to update. I feel like this change was badly planned and should be in a major release and not a minor one.

In my (and other commenters) view, this change is a backwards step that makes using molecule more difficult to use. I am failing to see the benefit of such a change.

evgeni added a commit to theforeman/foreman-operations-collection that referenced this pull request Feb 12, 2025
25.2.0 breaks discovery of role paths, see [1]

[1] ansible/molecule#4380
robin-checkmk added a commit to Checkmk/ansible-collection-checkmk.general that referenced this pull request Feb 12, 2025
evgeni added a commit to theforeman/foreman-operations-collection that referenced this pull request Feb 13, 2025
25.2.0 breaks discovery of role paths, see [1]

[1] ansible/molecule#4380
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.