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

lib/ansible/playbook/play.py

@@ -233,6 +233,10 @@ class Play(Base, Taggable, Become):
 
         if len(self.roles) > 0:
             for r in self.roles:
+                # Don't insert tasks from ``import/include_role``, preventing
+                # duplicate execution at the wrong time
+                if r.from_include:
+                    continue
                 block_list.extend(r.compile(play=self))
 
         return block_list

lib/ansible/playbook/role/__init__.py

@@ -96,7 +96,7 @@ class Role(Base, Become, Conditional, Taggable):
     _delegate_to = FieldAttribute(isa='string')
     _delegate_facts = FieldAttribute(isa='bool', default=False)
 
-    def __init__(self, play=None, from_files=None):
+    def __init__(self, play=None, from_files=None, from_include=False):
         self._role_name = None
         self._role_path = None
         self._role_params = dict()
@@ -108,6 +108,7 @@ class Role(Base, Become, Conditional, Taggable):
         self._dependencies = []
         self._task_blocks = []
         self._handler_blocks = []
+        self._compiled_handler_blocks = None
         self._default_vars = dict()
         self._role_vars = dict()
         self._had_task_run = dict()
@@ -117,6 +118,9 @@ class Role(Base, Become, Conditional, Taggable):
             from_files = {}
         self._from_files = from_files
 
+        # Indicates whether this role was included via include/import_role
+        self.from_include = from_include
+
         super(Role, self).__init__()
 
     def __repr__(self):
@@ -126,7 +130,7 @@ class Role(Base, Become, Conditional, Taggable):
         return self._role_name
 
     @staticmethod
-    def load(role_include, play, parent_role=None, from_files=None):
+    def load(role_include, play, parent_role=None, from_files=None, from_include=False):
 
         if from_files is None:
             from_files = {}
@@ -153,7 +157,7 @@ class Role(Base, Become, Conditional, Taggable):
                             role_obj.add_parent(parent_role)
                         return role_obj
 
-            r = Role(play=play, from_files=from_files)
+            r = Role(play=play, from_files=from_files, from_include=from_include)
             r._load_role_data(role_include, parent_role=parent_role)
 
             if role_include.role not in play.ROLE_CACHE:
@@ -359,7 +363,15 @@ class Role(Base, Become, Conditional, Taggable):
         return self._task_blocks[:]
 
     def get_handler_blocks(self, play, dep_chain=None):
-        block_list = []
+        # Do not recreate this list each time ``get_handler_blocks`` is called.
+        # Cache the results so that we don't potentially overwrite with copied duplicates
+        #
+        # ``get_handler_blocks`` may be called when handling ``import_role`` during parsing
+        # as well as with ``Play.compile_roles_handlers`` from ``TaskExecutor``
+        if self._compiled_handler_blocks:
+            return self._compiled_handler_blocks
+
+        self._compiled_handler_blocks = block_list = []
 
         # update the dependency chain here
         if dep_chain is None:

lib/ansible/playbook/role_include.py

@@ -46,7 +46,7 @@ class IncludeRole(TaskInclude):
 
     BASE = ('name', 'role')  # directly assigned
     FROM_ARGS = ('tasks_from', 'vars_from', 'defaults_from')  # used to populate from dict in role
-    OTHER_ARGS = ('apply', 'private', 'allow_duplicates')  # assigned to matching property
+    OTHER_ARGS = ('apply', 'private', 'public', 'allow_duplicates')  # assigned to matching property
     VALID_ARGS = tuple(frozenset(BASE + FROM_ARGS + OTHER_ARGS))  # all valid args
 
     # =================================================================================
@@ -55,6 +55,7 @@ class IncludeRole(TaskInclude):
     # private as this is a 'module options' vs a task property
     _allow_duplicates = FieldAttribute(isa='bool', default=True, private=True)
     _private = FieldAttribute(isa='bool', default=None, private=True)
+    _public = FieldAttribute(isa='bool', default=False, private=True)
 
     def __init__(self, block=None, role=None, task_include=None):
 
@@ -81,9 +82,13 @@ class IncludeRole(TaskInclude):
         ri.vars.update(self.vars)
 
         # build role
-        actual_role = Role.load(ri, myplay, parent_role=self._parent_role, from_files=self._from_files)
+        actual_role = Role.load(ri, myplay, parent_role=self._parent_role, from_files=self._from_files,
+                                from_include=True)
         actual_role._metadata.allow_duplicates = self.allow_duplicates
 
+        if self.statically_loaded or self.public:
+            myplay.roles.append(actual_role)
+
         # save this for later use
         self._role_path = actual_role._role_path
 
@@ -119,9 +124,12 @@ class IncludeRole(TaskInclude):
         if ir._role_name is None:
             raise AnsibleParserError("'name' is a required field for %s." % ir.action, obj=data)
 
-        if ir.private is not None:
+        if 'public' in ir.args and ir.action != 'include_role':
+            raise AnsibleParserError('Invalid options for %s: private' % ir.action, obj=data)
+
+        if 'private' in ir.args:
             display.deprecated(
-                msg='Supplying "private" for "include_role" is a no op, and is deprecated',
+                msg='Supplying "private" for "%s" is a no op, and is deprecated' % ir.action,
                 version='2.8'
             )