Skip to content

role arg spec - allow file extensions in tasks_from #75467

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

Draft
wants to merge 7 commits into
base: devel
Choose a base branch
from

Conversation

samdoran
Copy link
Contributor

@samdoran samdoran commented Aug 11, 2021

SUMMARY

Lookup the spec using the basename of the last part of the path or file specified in tasks_from. Since tasks_form accepts a file or path both with and without an extension, the argument spec lookup should as well.

Fixes #75456

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/playbook/role/__init__.py

To do:

  • Add tests

Lookup the spec using the basename of the file specified in tasks_from. Since
tasks_form accepts both with and without extensions, so should the argument
spec lookup.
@samdoran samdoran changed the title role arg spec - allow file extensions in task_from role arg spec - allow file extensions in tasks_from Aug 11, 2021
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.12 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Aug 11, 2021
This allows for relative paths, absolute paths, and files with or withou
an extension.
Change variable name to better reflect its contents.
@samdoran
Copy link
Contributor Author

I created a stem() function. This may make it unable to backport, but I can use the previous solution in the backport if we don't want to backport the new function.

@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Aug 11, 2021
Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>
@samdoran samdoran marked this pull request as ready for review August 12, 2021 15:45
@samdoran samdoran requested review from sivel and bcoca August 12, 2021 15:45
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Aug 12, 2021
@sivel
Copy link
Member

sivel commented Aug 12, 2021

I do question whether we should have a list of candidates to try. We might just say this isn't supported, but here it is.

Consider the following tree:

.
└── roles
    └── foo
        └── tasks
            ├── install.yml
            └── ubuntu
                └── install.yml

What about calculating the relative path based on role_path? And looping from most specific to least specific?

That way if someone provided tasks_from: ubuntu/install.yml it could have a different entry point than tasks_from: install.yml?

Just a thought, based on the current solution being naive about paths. Although this PR is an improvement from one view, it actually takes away the potential features.

Again, maybe this is just something we document as not being possible.

@bcoca
Copy link
Member

bcoca commented Aug 12, 2021

@sivel, iirc, tasks_from does not allow subdirs

@sivel
Copy link
Member

sivel commented Aug 12, 2021

It works fine with subdirs, I tested it before I posted my last comment.

@samdoran
Copy link
Contributor Author

That way if someone provided tasks_from: ubuntu/install.yml it could have a different entry point than tasks_from: install.yml?

In general, I do like this idea. The current behavior can be a bit surprising since it's pretty flexible/naive with its path searching.

One concern I have is how we would then match the role arg spec to the correct entry point if the stem is the same but the relative path is different. It seems like that would complicate the arg spec names and matching quite a bit.

Naming the spec install becomes ambiguous (though it already is). Would it need to be named based on the relative path inside tasks/? Something like install and ubunutu_install?

@sivel
Copy link
Member

sivel commented Aug 12, 2021

Naming the spec install becomes ambiguous (though it already is). Would it need to be named based on the relative path inside tasks/? Something like install and ubunutu_install?

I was thinking literally install and ubuntu/install to basically align with the functionality already achievable prior to this PR.

@nitzmahone nitzmahone removed the needs_triage Needs a first human triage before being processed. label Aug 12, 2021
@@ -217,3 +219,13 @@ def unquote(data):
if is_quoted(data):
return data[1:-1]
return data


def stem(path):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per discussion, you might want to look at is_subpath (ansible.utils.path) and also put this in common/file.py or create a common/path.py?

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 25, 2021
@samdoran samdoran marked this pull request as draft August 26, 2021 18:52
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Aug 26, 2021
@samdoran
Copy link
Contributor Author

I have been looking into this more and found that subdirs for tasks_from work with include_role but not import_role. I need to track down and reconcile that behavior.

 0.1s _load_role_data: self._from_files.get('tasks')='ubuntu/install'  # include_tasks

 0.1s _load_role_data: self._from_files.get('tasks')='install'         # import_tasks

The more I think about it, I do like the flexibility of having specs match the tasks_from hierarchy. I'm a bit leery of how tricky it will be to implement that since the task loading/finding code is separate from the spec finding code/logic.

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Mar 10, 2022
@Veratil
Copy link

Veratil commented Jul 15, 2022

I have been looking into this more and found that subdirs for tasks_from work with include_role but not import_role. I need to track down and reconcile that behavior.

 0.1s _load_role_data: self._from_files.get('tasks')='ubuntu/install'  # include_tasks

 0.1s _load_role_data: self._from_files.get('tasks')='install'         # import_tasks

The more I think about it, I do like the flexibility of having specs match the tasks_from hierarchy. I'm a bit leery of how tricky it will be to implement that since the task loading/finding code is separate from the spec finding code/logic.

https://github.com/ansible/ansible/blob/devel/lib/ansible/playbook/role_include.py#L154

Remove basename here. I wrote some tests and tested on devel and your branch, all passed. Validation is still run on the path and will fail when it's out of the role folder.

@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. has_issue and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jul 12, 2023
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Oct 24, 2023
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.12 bug This issue/PR relates to a bug. has_issue needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_pr This PR has not been pushed to for more than one year. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

role argspec validation doesn't run unless tasks_from entrypoint matches exactly
7 participants