-
Notifications
You must be signed in to change notification settings - Fork 205
fix: Make relative imports in collection modules work #1492
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -317,7 +317,7 @@ class NewStylePlanner(ScriptPlanner): | |
| preprocessing the module. | ||
| """ | ||
| runner_name = 'NewStyleRunner' | ||
| MARKER = re.compile(br'from ansible(?:_collections|\.module_utils)\.') | ||
| MARKER = re.compile(br'from ansible(?:_collections|\.module_utils)\.|from \.\.?(?:\w|\s)') | ||
|
|
||
| @classmethod | ||
| def detect(cls, path, source): | ||
|
|
@@ -653,6 +653,53 @@ def _fix_dnf(invocation, module_source): | |
| invocation._overridden_sources[invocation.module_path] = module_source | ||
|
|
||
|
|
||
| def _fix_collection_relative_imports(invocation, module_source): | ||
| """ | ||
| Collection modules using relative imports (from ..module_utils import ...) | ||
| fail because the ansible_collections namespace isn't set up. | ||
| Replace relative imports with absolute ansible_collections imports. | ||
| """ | ||
| if 'ansible_collections' not in invocation.module_path: | ||
| return | ||
|
|
||
| # Derive the collection's base package from the module path | ||
| # e.g. .../ansible_collections/hetzner/hcloud/plugins/modules/foo.py | ||
| # -> ansible_collections.hetzner.hcloud.plugins | ||
| parts = invocation.module_path.split(os.sep) | ||
| try: | ||
| ac_idx = parts.index('ansible_collections') | ||
| except ValueError: | ||
| return | ||
|
|
||
| # Need at least ansible_collections/{namespace}/{collection} | ||
| if len(parts) < ac_idx + 3: | ||
| return | ||
|
|
||
| module_package_parts = parts[ac_idx:parts.index(parts[-1])] | ||
| module_package = '.'.join(module_package_parts) | ||
|
|
||
| def resolve_relative_import(match): | ||
| dots = match.group(1) | ||
| package_name = match.group(2) | ||
| level = len(dots) | ||
|
|
||
| base_parts = module_package.split('.') | ||
| base_parts = base_parts[:len(base_parts) - level + 1] | ||
|
|
||
| if package_name: | ||
| absolute_package_name = ('from %s.%s' % ('.'.join(base_parts), package_name.decode())).encode() | ||
| else: | ||
| absolute_package_name = ('from %s' % '.'.join(base_parts)).encode() | ||
|
|
||
| return absolute_package_name | ||
|
|
||
| pattern = re.compile(b'from (\.\.?)\s?(\w+)') | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be a raw byte-string for the backslashed characters, as written. |
||
| fixed = pattern.sub(resolve_relative_import, module_source) | ||
|
|
||
| if fixed != module_source: | ||
| invocation._overridden_sources[invocation.module_path] = fixed | ||
|
|
||
|
|
||
| def _load_collections(invocation): | ||
| """ | ||
| Special loader that ensures that `ansible_collections` exist as a module path for import | ||
|
|
@@ -691,6 +738,7 @@ def invoke(invocation): | |
| module_source = invocation.get_module_source() | ||
| _fix_py35(invocation, module_source) | ||
| _fix_dnf(invocation, module_source) | ||
| _fix_collection_relative_imports(invocation, module_source) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note to self: I think (80% confidence) these fixup functions only handle Ansible modules, in other words they don't process the contents of module_utils files. That's a pre-existing limitatation. TIL: Some Ansible collections contain plugins/plugin_utils. I don't think this is a standardised collection dir, more a convention used by some authors to share code between plugins con the controller without exposing it to targets/modules. Seem to be more common in Microsoft collections Refs |
||
| _planner_by_path[invocation.module_path] = _get_planner( | ||
| invocation, | ||
| module_source | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue #896 mentions explicit-relative imports (e.g.
from ..module_utils.core import AnsibleAWSModule)that<some_prefix>.module_utils.<...>This regex as written would also match imports that
e.g. from .module_utils<...>)<some_prefix>.module_utils<...>Did you mean to cast such a wide net? What other relative imports is this meant to tackle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some rough grepping shows relative imports other than
from ..module_utilsare common in Ansible modules and module_utils. So question withdrawn.Details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but
_fix_collection_relative_imports()still only handlesfrom ..module_utils. So question reinstated: Why have you made MARKER fire on e.g.from .foo import bar, when the new handler only deals withfrom ..module_utils <...>?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. I had a little help from Copilot. I didn't catch that the replacement didn't make sense. I adjusted the code so it now resolves the package path correctly instead of just replacing
..module_utils.And yes, the idea is, that it resolves all relative imports:
from ..something import ...from .something import ...from . import ...I'd rather have a whole solution instead of patching things each time, some collection uses relative imports and stops working. I hope you support that.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, it doesn't handle 3 or more relative levels (
from ...foo import bar), which there are cases of in a default Ansible install.Edit: many cases in various module_utils. Exactly one case in a module shipped with Ansible 13.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On closer investigation, it may make more sense to rip out
ansible_mitogen.planner.NewStylePlanner.MARKERand useansible.executor.module_common.NEW_STYLE_PYTHON_MODULE_REinstead.