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 feature to expose vars/defaults with include/import_role #41330

Merged
merged 8 commits into from
Jul 15, 2018

Conversation

sivel
Copy link
Member

@sivel sivel commented Jun 8, 2018

SUMMARY

This PR aims to add functionality to import_role and include_role to allow exposing vars/defaults from the role.

import_role always exposes. include_role can be toggled, but defaults to public=False.

This PR re-uses the private argument, but I debate flipping that to public, or at minimum renaming to a different argument as to cause less confusion.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/playbook/helpers.py
lib/ansible/playbook/role_include.py

ANSIBLE VERSION
2.7
ADDITIONAL INFORMATION

@sivel
Copy link
Member Author

sivel commented Jun 8, 2018

not_bot_skip_any_more

@sivel sivel added this to Triage in include and import issues via automation Jun 8, 2018
@sivel sivel changed the title [WIP] Add feature to expose vars/defaults include/import_role [WIP] Add feature to expose vars/defaults with include/import_role Jun 8, 2018
@sivel
Copy link
Member Author

sivel commented Jun 8, 2018

TODO:

  • validate vars_from and how/where it occurs in regards to forking.
    • tests show vars/defaults_from is working, these are loaded just like vars in _load_role_data

@sivel sivel moved this from Triage to In Progress in include and import issues Jun 8, 2018
@@ -355,8 +355,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h
ir._role_name = templar.template(ir._role_name)

# uses compiled list from object
blocks, _ = ir.get_block_list(variable_manager=variable_manager, loader=loader)
task_list.extend(blocks)
ir.get_block_list(variable_manager=variable_manager, loader=loader)
Copy link
Member Author

@sivel sivel Jun 8, 2018

Choose a reason for hiding this comment

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

Without this change, the tasks get executed twice. Using this change causes the tasks of the role to execute before other tasks listed in tasks.

@sivel sivel force-pushed the import-include-role-private branch from d3f9ea9 to a5726ce Compare June 11, 2018 21:50
@samdoran
Copy link
Contributor

One thing I'm wondering about is the timing of variable availability. Are role variables only available after the role has been called or immediately from the beginning of the play? I would think this is the case for import_role but maybe included_rule with public: yes, the vars are only avaiable after that role has been included since it's dynamic.

Also, are the vars available to subsequent plays in the same playbook?

I just thought these would be useful things to clarify in the final docs and possibly implement in the final code.

@sivel
Copy link
Member Author

sivel commented Jun 12, 2018

One thing I'm wondering about is the timing of variable availability. Are role variables only available after the role has been called or immediately from the beginning of the play?

At the moment, there are tests describing this. import_role exposes at parse time, include_role exposes at execution time. So with import_role they are available before the tasks within execute, with include_role they only become available once the tasks within start executing.

Once, exposed, the same rules apply to roles vars that are defined under the roles: header.

@sivel sivel force-pushed the import-include-role-private branch from c45440e to a702926 Compare June 18, 2018 16:38
@sivel sivel changed the title [WIP] Add feature to expose vars/defaults with include/import_role Add feature to expose vars/defaults with include/import_role Jun 25, 2018
@sivel
Copy link
Member Author

sivel commented Jun 25, 2018

I think this can come out of WIP now.

@sivel sivel requested a review from bcoca June 25, 2018 21:05
@sivel sivel force-pushed the import-include-role-private branch from 4123cc8 to c7c2ded Compare July 15, 2018 13:50
@sivel
Copy link
Member Author

sivel commented Jul 15, 2018

(rebased)

@sivel sivel merged commit 27b4d7e into ansible:devel Jul 15, 2018
include and import issues automation moved this from In Progress to Done Jul 15, 2018
kiorky pushed a commit to corpusops/ansible that referenced this pull request Jul 25, 2018
…#41330)

* First pass at making 'private' work on include_role, imports are always public

* Prevent dupe task execution and overwriting handlers

* New functionality will use public instead of deprecated private

* Add tests for public exposure

* Validate vars before import/include to ensure they don't expose too early

* Add porting guide docs about public argument and change to import_role

* Add additional docs about public and vars exposure to module docs

* Insert role handlers at parse time, exposing them globally
@danihodovic
Copy link
Contributor

danihodovic commented Sep 5, 2018

This is awesome. My favorite feature of 2.7.0 Thanks @sivel

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants