net_template.py: Fix j2 file search path #15134

keithnoguchi · 2016-03-24T06:13:24Z

ISSUE TYPE

Bugfix Pull Request

ANSIBLE VERSION

air$ ansible-playbook --version
ansible-playbook 2.1.0
  config file =
  configured module search path = Default w/o overrides

SUMMARY

Fixes #15133

air$ play site.yaml

PLAY [fabric switches] *********************************************************

TASK [setup] *******************************************************************
ok: [fab1]

TASK [switch : print JSON input for this play] *********************************
skipping: [fab1]

TASK [switch : configure the switch] *******************************************
changed: [fab1]

TASK [switch : result from the switch] *****************************************
skipping: [fab1]

PLAY [spine switches] **********************************************************

TASK [setup] *******************************************************************
ok: [spine2]
ok: [spine1]

TASK [switch : print JSON input for this play] *********************************
skipping: [spine1]
skipping: [spine2]

TASK [switch : configure the switch] *******************************************
changed: [spine1]
changed: [spine2]

TASK [switch : result from the switch] *****************************************
skipping: [spine1]
skipping: [spine2]

PLAY [leaf switches] ***********************************************************

TASK [setup] *******************************************************************
ok: [leaf1]
ok: [leaf2]
ok: [leaf3]

TASK [switch : print JSON input for this play] *********************************
skipping: [leaf1]
skipping: [leaf3]
skipping: [leaf2]

TASK [switch : configure the switch] *******************************************
changed: [leaf3]
changed: [leaf1]
changed: [leaf2]

TASK [switch : result from the switch] *****************************************
skipping: [leaf2]
skipping: [leaf1]
skipping: [leaf3]

PLAY RECAP *********************************************************************
fab1                       : ok=2    changed=1    unreachable=0    failed=0
leaf1                      : ok=2    changed=1    unreachable=0    failed=0
leaf2                      : ok=2    changed=1    unreachable=0    failed=0
leaf3                      : ok=2    changed=1    unreachable=0    failed=0
spine1                     : ok=2    changed=1    unreachable=0    failed=0
spine2                     : ok=2    changed=1    unreachable=0    failed=0

This allows to '{% include ['another.j2'] %}' work inside j2 file.

coveralls · 2016-03-24T06:53:22Z

Coverage remained the same at 64.655% when pulling e8192a0b52bb5899d76cfed0afacb05a8211c7c2 on keinohguchi:net_template_fix into 85cc3b7 on ansible:devel.

keithnoguchi · 2016-03-24T07:01:23Z

travis is green, as shown below:

https://travis-ci.org/keinohguchi/ops-switch-role

bcoca · 2016-03-24T13:52:35Z

lib/ansible/plugins/action/net_template.py

@@ -93,6 +94,11 @@ def _handle_template(self):
        except IOError:
            return dict(failed=True, msg='unable to load src file')

+        searchpath = [working_path, os.path.dirname(source)]
+        if self._task._role is not None:
+            searchpath.insert(1, C.DEFAULT_ROLES_PATH)


DEFAULT_ROLES_PATH should never be in use by a plugin, it should always use the 'current role path' if it exists or fallback to the playbook path. Another issue is that this is treated as a single directory entry when it could be a list of paths.

Thank you @bcoca for your quick reply! I've actually followed the existing template.py as a sample. Let me know if the issue above also exists in template.py or net_template.py is another beast which we need to play differently.

https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/action/template.py#L125

Thank you for your support!

Cheers,

yep, that is wrong also, going to fix that

Awesome! I'll use yours as a reference! Thanks again!

https://github.com/ansible/ansible/pull/15145/files

keithnoguchi · 2016-03-24T23:41:03Z

Incorporate @bcoca's comment, but needs to use ._role_path, as get_dep_chain() returns Role object.

It worked on my environment but needs pro's review, as I don't have a sample dependency block playbooks...

I can test in my environment if you can give me sample dependency block, which exercise the particular code block.

Thank you for your support!

keithnoguchi · 2016-03-25T18:39:55Z

Oops, I shouldn't merge the submodule dependency…

I'll fix that and get back here.

Cheers,

keithnoguchi · 2016-03-28T22:37:33Z

Removed unnessesary change for the submodule git hash tag. Now the change only exists in net_template.py.

Please review!

keithnoguchi · 2016-04-21T15:29:59Z

Based on the IRC chat today, this might be the one to address the issue generically. Having a link here
so that, I can check it easily. :)

#15145

abadger · 2016-05-03T19:15:50Z

@bcoca, Any word on the generic fix for this? If not, we wondered at today's meeting if we should merge this as a short-term fix until the generic fix can be finished and merged.

bcoca · 2016-05-03T21:24:23Z

need time to finish it, too many things ... not sure if this fix is correct, it looks like one of my early attempts. As long as someone can verify the paths are correct, i'm fine with 'temp merge'

keithnoguchi · 2016-05-04T03:54:01Z

Thanks @bcoca for the reply! We'll discuss about this during tomorrow's network meeting and will have a consensus among the team.

Cheers and have a wonderful night!

keithnoguchi · 2016-05-07T18:12:39Z

Hi @Qalthos,

Can you take a look at this PR, as suggested by @privateip durinug last network meeting?

networking_working_group.2016-05-04-16.03.html

I should've done this but waited to get green by travis.... It seems that's common issue across the board.

Thank you for your support!

Cheers,

keithnoguchi · 2016-05-10T20:58:44Z

Hi @Qalthos,

I've rebased the branch against the latest devel, as it has a fix for the travis CI test case. Once we get
the green sign from Travis, I believe we're ready to go.

Thank you for your support and hope this will be in to the 2.1. :)

Cheers,

keithnoguchi · 2016-05-12T16:19:19Z

Thank you @jim-c for taking care of those labels!

I've just rebased the branch just now and kicked the travis. Hope we'll get a green sign from him. :)

Cheers!

Qalthos · 2016-05-12T16:21:14Z

lib/ansible/plugins/action/net_template.py

+            if self._task._block._dep_chain:
+                for role in sorted(self._task._block.get_dep_chain()):
+                    searchpath.insert(1, role._role_path)
+            searchpath.insert(1, self._task._role._role_path)


I'm trying to follow the final order of the paths... I think the final order is [working _path, _role._role_path, *_block._role_paths, dirname(source)]

Would it not be more straightforward to order it thusly:

searchpath = [working_path] if self._task._role is not None: searchpath.append(_role._role_path) for role in sorted(dep_chain()): searchpath.append(role._role_path) searchpath.append(dirname(source))

That would seem to make the intended order clearer while avoiding the overhead of a bunch of insert(1, x) calls

Thank you @Qalthos and I agree with that suggestion.

I've incorporated your suggestion and run it locally, without any issue.

Please take a look at the updated change and let me know if you see any other issues.

Thank you for your support and hope we'll get the green sign from travis. :)

Cheers,

The change is needed to support the multiple include statements inside the jinja2 template file, as in '{% include ['another.j2'] %}'. statement. I need this capability, as OpenSwitch `switch` role needs to handle multiple *.j2 files and supporting the include statement inside jinja2 file is essential, otherwise I need to combine multiple template files into a single file, which easily causes conflicts between developers working on different parts of the teamplate, ports and interface.

keithnoguchi · 2016-05-13T21:49:23Z

Hi @Qalthos,

I've update the patch, to use the get_dep_chain() as well as removing the sort(), as we don't need the
sorting on the dependent roles. I've modified my bgp role, to have a switch role as a dependent, and run the bgp only playbook, which execute the switch role play as a depenent play, without having any template path error.

Please take a look at the updated patch.

Cheers,

The change is needed to support the multiple include statements inside the jinja2 template file, as in '{% include ['another.j2'] %}'. statement. I need this capability, as OpenSwitch `switch` role needs to handle multiple *.j2 files and supporting the include statement inside jinja2 file is essential, otherwise I need to combine multiple template files into a single file, which easily causes conflicts between developers working on different parts of the teamplate, ports and interface.

keithnoguchi · 2016-05-16T20:08:51Z

Thank you @Qalthos and the team for your support and make it happen for 2.1!

Really appreciated, team!

I can the devel branch for my testing finally! :)

Cheers and happy 2.1! :)

bcoca reviewed Mar 24, 2016
View reviewed changes

bcoca added bugfix_pull_request needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 28, 2016

keithnoguchi mentioned this pull request Apr 20, 2016

Agenda for Public Core Meeting - 2016-04-21 @ 15:00 UTC ansible/community#83

Closed

abadger added this to the 2.1.0 milestone Apr 28, 2016

abadger added the P2 Priority 2 - Issue Blocks Release label Apr 28, 2016

keithnoguchi mentioned this pull request May 3, 2016

Agenda for Public Core Meeting - 2016-05-03 @ 19:00 UTC ansible/community#86

Closed

jimi-c assigned Qalthos May 12, 2016

jimi-c removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label May 12, 2016

Qalthos reviewed May 12, 2016
View reviewed changes

Qalthos merged commit 8de25db into ansible:devel May 16, 2016

ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 5, 2018

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net_template.py: Fix j2 file search path #15134

net_template.py: Fix j2 file search path #15134

keithnoguchi commented Mar 24, 2016

coveralls commented Mar 24, 2016

keithnoguchi commented Mar 24, 2016

bcoca Mar 24, 2016

keithnoguchi Mar 24, 2016

bcoca Mar 24, 2016

keithnoguchi Mar 24, 2016

keithnoguchi commented Mar 24, 2016

keithnoguchi commented Mar 25, 2016

keithnoguchi commented Mar 28, 2016

keithnoguchi commented Apr 21, 2016

abadger commented May 3, 2016

bcoca commented May 3, 2016

keithnoguchi commented May 4, 2016

keithnoguchi commented May 7, 2016

keithnoguchi commented May 10, 2016

keithnoguchi commented May 12, 2016

Qalthos May 12, 2016

keithnoguchi May 12, 2016

keithnoguchi commented May 13, 2016

keithnoguchi commented May 16, 2016

net_template.py: Fix j2 file search path #15134

net_template.py: Fix j2 file search path #15134

Conversation

keithnoguchi commented Mar 24, 2016

ISSUE TYPE

ANSIBLE VERSION

SUMMARY

coveralls commented Mar 24, 2016

keithnoguchi commented Mar 24, 2016

bcoca Mar 24, 2016

Choose a reason for hiding this comment

keithnoguchi Mar 24, 2016

Choose a reason for hiding this comment

bcoca Mar 24, 2016

Choose a reason for hiding this comment

keithnoguchi Mar 24, 2016

Choose a reason for hiding this comment

keithnoguchi commented Mar 24, 2016

keithnoguchi commented Mar 25, 2016

keithnoguchi commented Mar 28, 2016

keithnoguchi commented Apr 21, 2016

abadger commented May 3, 2016

bcoca commented May 3, 2016

keithnoguchi commented May 4, 2016

keithnoguchi commented May 7, 2016

keithnoguchi commented May 10, 2016

keithnoguchi commented May 12, 2016

Qalthos May 12, 2016

Choose a reason for hiding this comment

keithnoguchi May 12, 2016

Choose a reason for hiding this comment

keithnoguchi commented May 13, 2016

keithnoguchi commented May 16, 2016