Error if a module is found to shadow a reserved keyword

[sivel]

SUMMARY

Error if a module is found to shadow a reserved keyword

This prevents the error:

ERROR! conflicting action statements: command, tags

and replaces it with

ERROR! Module "tags" shadows the name of a reserved keyword. Please rename or remove this module. Found at /Users/matt/projects/ansibledev/playbooks/tags/library/tags

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

lib/ansible/plugins/loader.py

ANSIBLE VERSION

ADDITIONAL INFORMATION

[maxamillion]

Fix merge conflicts and then ship it! 👍

[sivel]

Rebased, but not sure if we can merge to 2.5. I'll check on that.

[sivel]

I will need to look into the failure. This worked in the past but fails now. Need to ensure this is limited to modules, and doesn't trigger on other plugins.

[alikins]

@sivel A variation on this idea would be to stop raising AnsibleError for this case in mod_args and wait until keyword validation to raise an error.

I have an old branch at devel...alikins:site_role_devel that did part of this, though for different reasons. The 'site_role_devel' branch was fixing errors that would happen if there were no (or not all) modules available when the playbooks are being parsed/loaded. For that case, mod_args.parse would always fail in the same place because there it wouldn't find a matching module for the task arg.

[alikins]

devel...alikins:site_role_devel was for the case of not finding a matching module at all, but I think a similar approach could work for possibly conflicting action/args as well. Since we don't really know all the valid modules until later (ie, that 'tags.json' isnt a valid module and does not conflict with 'tags' attribute)

[sivel]

I suppose that is possible. It depends on when we want this. If we refactor, we are waiting until 2.7. If this goes in now, it could be part of 2.5 potentially.

I'd be concerned though with doing this in _validate_attributes because that would cause us to potentially wait longer to see an error. I've never run into a problem where I didn't have the module I needed to execute the playbook. My personal feeling is that it shouldn't happen and should be an error.

(ie, that 'tags.json' isnt a valid module and does not conflict with 'tags' attribute)

Not sure I follow this. How would we know that? We don't restrict module file extensions? When would we be able to say that tags.json isn't a module? As far as I know, that can only be detected by trying to run it, and even then likely not in a way you might expect.

[alikins]

@sivel

I'd be concerned though with doing this in _validate_attributes because that would cause us to potentially wait longer to see an error. I've never run into a problem where I didn't have the module I needed to execute the playbook. My personal feeling is that it shouldn't happen and should be an error.

I think showing an error later is fine if it is a better error. To use an analogy to python, the current behavior feels like raising a SyntaxError when it should be raising a NameError.

Neither the no modules error or the conflicting action error should happen if the system configuration is correct. And currently not having any modules (or at least ping ) is a fatal error. The no modules due to misconfiguration is pretty common.

For the site_role_devel branch, the goal was to be able to have all modules provided by a role, so the error handling in mod args had to be defered to later until after roles were loaded.

Not sure I follow this. How would we know that? We don't restrict module file extensions? When would we be able to say that tags.json isn't a module? As far as I know, that can only be detected by trying to run it, and even then likely not in a way you might expect.

The main goal there is that we would avoid needing to look for modules at that point. mod_args.parse() happens very early and requires module_loader.find_plugin() to run early when it could be deferred to later after any library path changes have 'settled' (at least until dynamic includes start modifying it again).

[sivel]

I'm not going to go down that path now. I don't see a path to implement this that executes later without boiling the ocean right now.

I'm going to aim for the path of least resistance.

[mattclay]

Since this is limited to the ansible.modules package, is it able to catch any issues that would not be detected by a sanity test?

[sivel]

The purpose of this PR is not to resolve a CI related problem or to resolve issues with module submissions. It is however designed to solve issues our users face when they may have a file that is picked up as a module, that shadows the name of a keyword. There are 2 example issues linked to this PR that showcase the issue.

[mattclay]

The part I was missing here is that user's modules are also in the ansible.modules package. I was under the incorrect impression that they were in a separate package.