return changed for group_by (#32057) (#69860)

* return changed for group_by

* added tests and fixed 'early registeration'

(cherry picked from commit 22da903e9c)

changelogs/fragments/group_by_changed.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - group_by now should correctly refect changed status.

lib/ansible/inventory/data.py

@@ -255,19 +255,20 @@ class InventoryData(object):
 
     def add_child(self, group, child):
         ''' Add host or group to group '''
-
+        added = False
         if group in self.groups:
             g = self.groups[group]
             if child in self.groups:
-                g.add_child_group(self.groups[child])
+                added = g.add_child_group(self.groups[child])
             elif child in self.hosts:
-                g.add_host(self.hosts[child])
+                added = g.add_host(self.hosts[child])
             else:
                 raise AnsibleError("%s is not a known host nor group" % child)
             self._groups_dict_cache = {}
             display.debug('Group %s now contains %s' % (group, child))
         else:
             raise AnsibleError("%s is not a known group" % group)
+        return added
 
     def get_groups_dict(self):
         """

lib/ansible/inventory/group.py

@@ -169,7 +169,7 @@ class Group:
         return self.name
 
     def add_child_group(self, group):
-
+        added = False
         if self == group:
             raise Exception("can't add group to itself")
 
@@ -184,6 +184,7 @@ class Group:
             new_ancestors.add(self)
             new_ancestors.difference_update(start_ancestors)
 
+            added = True
             self.child_groups.append(group)
 
             # update the depth of the child
@@ -200,6 +201,7 @@ class Group:
                     h.populate_ancestors(additions=new_ancestors)
 
             self.clear_hosts_cache()
+        return added
 
     def _check_children_depth(self):
 
@@ -221,19 +223,24 @@ class Group:
                 raise AnsibleError("The group named '%s' has a recursive dependency loop." % to_native(self.name))
 
     def add_host(self, host):
+        added = False
         if host.name not in self.host_names:
             self.hosts.append(host)
             self._hosts.add(host.name)
             host.add_group(self)
             self.clear_hosts_cache()
+            added = True
+        return added
 
     def remove_host(self, host):
-
+        removed = False
         if host.name in self.host_names:
             self.hosts.remove(host)
             self._hosts.remove(host.name)
             host.remove_group(self)
             self.clear_hosts_cache()
+            removed = True
+        return removed
 
     def set_variable(self, key, value):
 

lib/ansible/inventory/host.py

@@ -113,7 +113,7 @@ class Host:
                     self.groups.append(group)
 
     def add_group(self, group):
-
+        added = False
         # populate ancestors first
         for oldg in group.get_ancestors():
             if oldg not in self.groups:
@@ -122,11 +122,14 @@ class Host:
         # actually add group
         if group not in self.groups:
             self.groups.append(group)
+            added = True
+        return added
 
     def remove_group(self, group):
-
+        removed = False
         if group in self.groups:
             self.groups.remove(group)
+            removed = True
 
             # remove exclusive ancestors, xcept all!
             for oldg in group.get_ancestors():
@@ -136,6 +139,7 @@ class Host:
                             break
                     else:
                         self.remove_group(oldg)
+        return removed
 
     def set_variable(self, key, value):
         if key in self.vars and isinstance(self.vars[key], MutableMapping) and isinstance(value, Mapping):

lib/ansible/plugins/strategy/__init__.py

@@ -528,16 +528,6 @@ class StrategyBase:
                     self._tqm.send_callback('v2_runner_item_on_ok', task_result)
                 continue
 
-            if original_task.register:
-                host_list = self.get_task_hosts(iterator, original_host, original_task)
-
-                clean_copy = strip_internal_keys(module_response_deepcopy(task_result._result))
-                if 'invocation' in clean_copy:
-                    del clean_copy['invocation']
-
-                for target_host in host_list:
-                    self._variable_manager.set_nonpersistent_facts(target_host, {original_task.register: clean_copy})
-
             # all host status messages contain 2 entries: (msg, task_result)
             role_ran = False
             if task_result.is_failed():
@@ -718,6 +708,18 @@ class StrategyBase:
                 self._pending_handler_results -= 1
             else:
                 self._pending_results -= 1
+
+            # register final results
+            if original_task.register:
+                host_list = self.get_task_hosts(iterator, original_host, original_task)
+
+                clean_copy = strip_internal_keys(module_response_deepcopy(task_result._result))
+                if 'invocation' in clean_copy:
+                    del clean_copy['invocation']
+
+                for target_host in host_list:
+                    self._variable_manager.set_nonpersistent_facts(target_host, {original_task.register: clean_copy})
+
             if original_host.name in self._blocked_hosts:
                 del self._blocked_hosts[original_host.name]
 
@@ -850,20 +852,20 @@ class StrategyBase:
         group = self._inventory.groups[group_name]
         for parent_group_name in parent_group_names:
             parent_group = self._inventory.groups[parent_group_name]
-            parent_group.add_child_group(group)
+            new = parent_group.add_child_group(group)
+            if new and not changed:
+                changed = True
 
-        if real_host.name not in group.get_hosts():
-            group.add_host(real_host)
-            changed = True
+        if real_host not in group.get_hosts():
+            changed = group.add_host(real_host)
 
-        if group_name not in host.get_groups():
-            real_host.add_group(group)
-            changed = True
+        if group not in real_host.get_groups():
+            changed = real_host.add_group(group)
 
         if changed:
             self._inventory.reconcile_inventory()
 
-        return changed
+        result_item['changed'] = changed
 
     def _copy_included_file(self, included_file):
         '''

test/integration/targets/group_by/create_groups.yml

@@ -20,6 +20,14 @@
 
 - name: group by genus
   group_by: key={{ genus }}
+  register: grouped_by_genus
+
+- debug: var=grouped_by_genus
+
+- name: ensure we reflect 'changed' on change
+  assert:
+    that:
+      - grouped_by_genus is changed
 
 - name: group by first three letters of genus with key in quotes
   group_by: key="{{ genus[:3] }}"