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 rel_file to ansible_managed template variable #69340

Closed
wants to merge 1 commit into from

Conversation

dmke
Copy link

@dmke dmke commented May 5, 2020

SUMMARY

This add a rel_file format string to the ansible_managed template variable.

Fixes #45654

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

template?

ADDITIONAL INFORMATION

The relative path resolution is very simplistic: it just strips the inventory_dir from the template_path, which may or may not be enough for this feature.

Here's a sample output of this patch applied to stable-2.9:

$ fgrep ansible_managed ./ansible.cfg
ansible_managed = Ansible managed: {rel_file} - DO NOT EDIT

$ cat playbooks/testbed.yml
---
- hosts: localhost
  roles:
  - testbed

$ cat roles/testbed/tasks/main.yml
---
- name: create file
  template:
    src: test.j2
    dest: /tmp/test.ansible

$ cat roles/testbed/templates/test.j2
{{ ansible_managed }}

$ ansible-playbook playbooks/testbed.yml --diff
PLAY [localhost] *************************************************************

TASK [Gathering Facts] *******************************************************
ok: [localhost]

TASK [testbed : create file] *************************************************
--- before
+++ after: /home/dm/.ansible/tmp/ansible-local-13670DFMU7R/tmpIK7AXO/test.j2
@@ -0,0 +1,1 @@
+# Ansible managed: roles/mm/templates/test.j2 - DO NOT EDIT

changed: [localhost]

PLAY RECAP *******************************************************************
localhost: ok=2 changed=1 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0
TODOs
  • Bike shedding: How should the variable be named? rel_file might not be descriptive enough.
  • Path resolution: Is the current implementation too naïve? If so, what are the environments I should consider? Is basing this off of inventory_dir the wrong approach?
  • Documentation: I have not yet touched the documentation. The rationale from "{rel_file}" in ansible_managed #45654 should be included.
  • Test: As far as I can tell, this is not heavily tested. I also haven't touched the tests yet.

I'm not a Python developer by trade, so this small patch might contain non-idiomatic code. I'm open for suggestions.

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. support:core This issue/PR relates to code supported by the Ansible Engineering Team. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels May 5, 2020
Copy link
Member

@bcoca bcoca left a comment

Choose a reason for hiding this comment

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

a couple of things
a) inventory_dir might not be present (see implicit localhost)
b) inventory is not the source for relative file paths, the 'play' is, if they happen to be the same in your setup that is coincidental, but inventory is not part of template/vars/files search path (it is for host_vars/group_vars, but that is diff system)

You might want to look at the dwim functions and how they use a search path, it is not as simple as you might think, specially when templates/ is optionally added and when roles appear in the mix, all create special cases.

@dmke
Copy link
Author

dmke commented May 6, 2020

a) inventory_dir might not be present (see implicit localhost)

I was not aware of this. However, if task_vars.get('inventory_dir') returns None, rel_path end up as being the same as path

b) inventory is not the source for relative file paths, the 'play' is, if they happen to be the same in your setup that is coincidental, but inventory is not part of template/vars/files search path

Indeed, I have inventory = ./hosts in my ansible.cfg (with both files checked into the repository).

You might want to look at the dwim functions and how they use a search path

Hm, grep'ing for dwim yields 80+ results, with path_dwim defined in lib/ansible/parsing/dataloader.py. Do you mean that class?

Alternatively, might a simple os.getcwd() suffice as base_dir for this purpose? Presumably, one would invoke ansible-playbook always from the same working directory? I mean, the use case for rel_file is a shared repository, which should be fully self-contained (i.e. containing one or more inventories, the Ansible configuration, playbooks, roles, and plugins, as this minimizes setup-time for each user interacting with that repo).

@bcoca
Copy link
Member

bcoca commented May 7, 2020

yes, dataloader has several dwim (do what i mean) functions for resolving relative paths, depending on context, you end up using one of those

cwd is not correct, you need basedir, since you can cd /var/tmp && ansible-playbook ~/projects/play.yml. We purposefully avoid using cwd when we can to avoid issues like that, we are always relative to either inventory, playbook or role.

@dmke
Copy link
Author

dmke commented May 7, 2020

Okay, thanks for the info.

Can you recommend a debugger (or anything more sophisticated than print()) to step through the code base for runtime introspection? As I said, I'm not a Python developer, and the Ansible code base is... a bit overwhelming :-)

@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label May 7, 2020
@bcoca
Copy link
Member

bcoca commented May 7, 2020

epdb is THE debugger, others exist (i do abuse print() but also the q library).

In any case I would use the original argument, which might already be a relative path, if not, compare to the task's base dir and remove the common sides from the left.

@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 May 22, 2020
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jun 15, 2020
@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. 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 Dec 8, 2020
@bcoca
Copy link
Member

bcoca commented Feb 2, 2022

Closing this as other modifications introduced template_fullpath leaving template_path as the 'unresolved' path supplied to the template, this could be relative or not, but results in more or less the same as this modification that relies on having a base_path or not.

@bcoca bcoca closed this Feb 2, 2022
@ansible ansible locked and limited conversation to collaborators Feb 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 feature This issue/PR relates to a feature request. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. pre_azp This PR was last tested before migration to Azure Pipelines. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"{rel_file}" in ansible_managed
3 participants