[PR #8510/21b16c1c backport][stable-9] Update VirtualBox Group parsing to align with documentation. (#8621)

Update VirtualBox Group parsing to align with documentation. (#8510)

* Update VirtualBox Group parsing to align with documentation.

Previously, we could separate the group string on the `/` char and
consider each element to be distinct, top-level groups. This change
implements the notion of nested groups and the use of the `,` char to
split multiple groups.

* Address code review comments.

Changed the implementation from a breaking change to a minor change by
introducing a new parameter to configure the behaviour. Keep the default
values to maintain the existing behaviour, and allow consumers an option
to opt-in.

* Fix line length.

The long lines were tripping CI. Reduce the length.

* Apply suggestions from code review

Update documentation to match expected conventions and correct the final rendered formatting.
Set the initial parent_group to `None` instead of `all` and rely on the parent class' inventory reconciliation logic to ensure consistent behaviour across different inventory plugins.

Co-authored-by: Felix Fontein <felix@fontein.de>

* Reword module arg description to avoid issues with CI.

One of the lines ended with a colon character which made the CI tests
fail since it would interpret it as a YAML key. Reworded the description
altogether to avoid that issue.

* Apply suggestions from code review

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 21b16c1c77)

Co-authored-by: lyrandy <42095565+lyrandy@users.noreply.github.com>

changelogs/fragments/8508-virtualbox-inventory.yml

@@ -0,0 +1,3 @@
+minor_changes:
+  - >- 
+    virtualbox inventory plugin - expose a new parameter ``enable_advanced_group_parsing`` to change how the VirtualBox dynamic inventory parses VM groups (https://github.com/ansible-collections/community.general/issues/8508, https://github.com/ansible-collections/community.general/pull/8510).

plugins/inventory/virtualbox.py

@@ -14,6 +14,8 @@ DOCUMENTATION = '''
         - Get inventory hosts from the local virtualbox installation.
         - Uses a YAML configuration file that ends with virtualbox.(yml|yaml) or vbox.(yml|yaml).
         - The inventory_hostname is always the 'Name' of the virtualbox instance.
+        - Groups can be assigned to the VMs using C(VBoxManage). Multiple groups can be assigned by using V(/) as a delimeter.
+        - A separate parameter, O(enable_advanced_group_parsing) is exposed to change grouping behaviour. See the parameter documentation for details.
     extends_documentation_fragment:
       - constructed
       - inventory_cache
@@ -35,6 +37,19 @@ DOCUMENTATION = '''
             description: create vars from virtualbox properties
             type: dictionary
             default: {}
+        enable_advanced_group_parsing:
+            description:
+              - The default group parsing rule (when this setting is set to V(false)) is to split the VirtualBox VM's group based on the V(/) character and
+                assign the resulting list elements as an Ansible Group.
+              - Setting O(enable_advanced_group_parsing=true) changes this behaviour to match VirtualBox's interpretation of groups according to
+                U(https://www.virtualbox.org/manual/UserManual.html#gui-vmgroups).
+                Groups are now split using the V(,) character, and the V(/) character indicates nested groups.
+              - When enabled, a VM that's been configured using V(VBoxManage modifyvm "vm01" --groups "/TestGroup/TestGroup2,/TestGroup3") will result in
+                the group C(TestGroup2) being a child group of C(TestGroup); and
+                the VM being a part of C(TestGroup2) and C(TestGroup3).
+            default: false
+            type: bool
+            version_added: 9.2.0
 '''
 
 EXAMPLES = '''
@@ -177,14 +192,10 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable):
 
             # found groups
             elif k == 'Groups':
-                for group in v.split('/'):
+                if self.get_option('enable_advanced_group_parsing'):
-                    if group:
+                    self._handle_vboxmanage_group_string(v, current_host, cacheable_results)
-                        group = make_unsafe(group)
+                else:
-                        group = self.inventory.add_group(group)
+                    self._handle_group_string(v, current_host, cacheable_results)
-                        self.inventory.add_child(group, current_host)
-                        if group not in cacheable_results:
-                            cacheable_results[group] = {'hosts': []}
-                        cacheable_results[group]['hosts'].append(current_host)
                 continue
 
             else:
@@ -227,6 +238,64 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable):
 
         return all(find_host(host, inventory))
 
+    def _handle_group_string(self, vboxmanage_group, current_host, cacheable_results):
+        '''Handles parsing the VM's Group assignment from VBoxManage according to this inventory's initial implementation.'''
+        # The original implementation of this inventory plugin treated `/` as
+        # a delimeter to split and use as Ansible Groups.
+        for group in vboxmanage_group.split('/'):
+            if group:
+                group = make_unsafe(group)
+                group = self.inventory.add_group(group)
+                self.inventory.add_child(group, current_host)
+                if group not in cacheable_results:
+                    cacheable_results[group] = {'hosts': []}
+                cacheable_results[group]['hosts'].append(current_host)
+
+    def _handle_vboxmanage_group_string(self, vboxmanage_group, current_host, cacheable_results):
+        '''Handles parsing the VM's Group assignment from VBoxManage according to VirtualBox documentation.'''
+        # Per the VirtualBox documentation, a VM can be part of many groups,
+        # and it's possible to have nested groups.
+        # Many groups are separated by commas ",", and nested groups use
+        # slash "/".
+        # https://www.virtualbox.org/manual/UserManual.html#gui-vmgroups
+        # Multi groups: VBoxManage modifyvm "vm01" --groups "/TestGroup,/TestGroup2"
+        # Nested groups: VBoxManage modifyvm "vm01" --groups "/TestGroup/TestGroup2"
+
+        for group in vboxmanage_group.split(','):
+            if not group:
+                # We could get an empty element due how to split works, and
+                # possible assignments from VirtualBox. e.g. ,/Group1
+                continue
+
+            if group == "/":
+                # This is the "root" group. We get here if the VM was not
+                # assigned to a particular group. Consider the host to be
+                # unassigned to a group.
+                continue
+
+            parent_group = None
+            for subgroup in group.split('/'):
+                if not subgroup:
+                    # Similarly to above, we could get an empty element.
+                    # e.g //Group1
+                    continue
+
+                if subgroup == '/':
+                    # "root" group.
+                    # Consider the host to be unassigned
+                    continue
+
+                subgroup = make_unsafe(subgroup)
+                subgroup = self.inventory.add_group(subgroup)
+                if parent_group is not None:
+                    self.inventory.add_child(parent_group, subgroup)
+                self.inventory.add_child(subgroup, current_host)
+                if subgroup not in cacheable_results:
+                    cacheable_results[subgroup] = {'hosts': []}
+                cacheable_results[subgroup]['hosts'].append(current_host)
+
+                parent_group = subgroup
+
     def verify_file(self, path):
 
         valid = False