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 warning when later roles are overridden #48587

Closed
wants to merge 1 commit into from

Conversation

@ianw
Copy link
Contributor

@ianw ianw commented Nov 13, 2018

SUMMARY

You currently don't get any sort of help diagnosing a situation where
a role in a later entry of ANSIBLE_ROLES_PATH is overriden by a role
with the same name earlier in the path. Although you can pick things
out of the task paths from when it's running, since the names are the
same (kind of the point :) it can be very confusing as to what's
happening.

This continues the path walk when importing the roles, and warns if
you have later roles that are being ignored due to earlier matches.

The documentation is enhanced to discuss the role import precedence.

An integration test is added to match the new behaviour.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

playbook/role

ADDITIONAL INFORMATION

This situation arose because we moved a role in our git tree. This particular role had a filter_plugin so it actually left behind a __pycache__ directory and .pyc file from when it had run. This meant we had essentially a blank role -- no tasks, variables or anything else in the role -- but it was still being picked up and "run". With no tasks it just appeared that the role was being read (I mean, the role: -roleinquestion was not giving a not found error) but somehow doing nothing at all. This was even more vexing, as "git status" usually ignores .pyc files so it was completely opaque as to what was going on, and it had only moved up a directory so you can very easily miss this in long paths.

Of course, when it is explained, it all becomes rather obvious. However, I feel a warning like this would have made it much clearer much sooner. I think a warning is justified as it is best practice to avoid pointing guns at your foot by giving roles the same name.

@ansibot
Copy link
Contributor

@ansibot ansibot commented Nov 13, 2018

Hi @ianw, thank you for submitting this pull-request!

click here for bot help

@ansibot
Copy link
Contributor

@ansibot ansibot commented Nov 13, 2018

@ansibot
Copy link
Contributor

@ansibot ansibot commented Nov 13, 2018

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

test/integration/targets/include_import/override_role/role1/tasks/main.yml:3:1: empty-lines too many blank lines (1 > 0)

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Nov 13, 2018
@ianw ianw force-pushed the ianw:note-role-overrides branch Nov 13, 2018
@bcoca bcoca removed the needs_triage label Nov 13, 2018
@bcoca bcoca assigned bcoca and sivel Nov 13, 2018
@mattclay
Copy link
Member

@mattclay mattclay commented Nov 16, 2018

@ianw
Copy link
Contributor Author

@ianw ianw commented Nov 16, 2018

@mattclay would you have any suggestions here? When I run ./bin/ansible-test integration --tox include_import it seems to work

@ansibot ansibot added the stale_ci label Nov 24, 2018
@bcoca
Copy link
Member

@bcoca bcoca commented Nov 30, 2018

This seems expensive to have in runtime, ansible-galaxy list will show duplicate roles in different paths

@ianw ianw force-pushed the ianw:note-role-overrides branch Nov 30, 2018
You currently don't get any sort of help diagnosing a situation where
a role in a later entry of ANSIBLE_ROLES_PATH is overriden by a role
with the same name earlier in the path.  Although you can pick things
out of the task paths from when it's running, since the names are the
same (kind of the point :) it can be very confusing as to what's
happening.

This continues the path walk when importing the roles, and warns if
you have later roles that are being ignored due to earlier matches.

The documentation is enhanced to discuss the role import precedence.

An integration test is added to match the new behaviour.
@ianw ianw force-pushed the ianw:note-role-overrides branch to 2438834 Nov 30, 2018
@ianw ianw closed this Nov 30, 2018
@ianw ianw reopened this Nov 30, 2018
@jimi-c
Copy link
Member

@jimi-c jimi-c commented Jun 5, 2020

Hi @ianw, sorry for taking so long to get back to this. Unfortunately I think we're going to pass on merging this for now for the following reasons:

  • Collections help to avoid collisions, so using the FQCN for roles should prevent the problem.
  • Using verbose output shows the path to the task as it's run, so while debugging issues you can see exactly where the task definition came from including the role path.
  • Due to the addition of collections, additional checks would need to be made while looking up the role path.

If you have any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular issue is resolved.

Thank you!

@jimi-c jimi-c closed this Jun 5, 2020
@ansible ansible locked and limited conversation to collaborators Jul 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.