Update VirtualBox Group parsing to align with documentation.

[lyrandy]

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.

SUMMARY

Fixes #8508
Update the parsing algorithm in the VirtualBox dynamic inventory. The update changes how groups are formed when considering VirtualBox VMs.

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

virtualbox inventory plugin

ADDITIONAL INFORMATION

Before, we would split groups based only on the / characters to form top-level groups. This was problematic for users (like me) who want to use nested groups for better variable management.

This change introduces the ability to use nested groups using /, and different groups using ,. This approach more closely aligns with the official VirtualBox documentation.

https://www.virtualbox.org/manual/UserManual.html#gui-vmgroups

In the linked issue, it shows that previously, the my_custom_variable would resolve to "This is in \"zgroup1\" group" because the groups for vm-1 are on "the same level", and so variable resolution depends on lexicographical ordering.
Also, groups for vm-2 had a trailing , character.

Here are the results of the Ansible debug commands with this updated inventory file.

ansible -i virtualbox.yml -m debug -a "var=my_custom_variable" all
vm-1 | SUCCESS => {
    "my_custom_variable": "This is in \"xgroup3\" group"
}
vm-2 | SUCCESS => {
    "my_custom_variable": "This is in \"all\" group"
}

ansible -i virtualbox.yml -m debug -a "var=group_names" all
vm-1 | SUCCESS => {
    "group_names": [
        "xgroup3",
        "ygroup2",
        "zgroup1"
    ]
}
vm-2 | SUCCESS => {
    "group_names": [
        "othergroup1",
        "othergroup2",
        "othergroup3"
    ]
}

[felixfontein]

Thanks for your contribution!

I'm pretty sure this is a new feature, and not a bugfix, since the documentation of the inventory plugin never mentioned the new behavior. It is a new feature, and (as you correctly stated in the changelog fragment) a breaking change. Breaking changes are not acceptable without a long enough deprecation period informing users of the changing behavior.

The best choice is to make the behavior configurable, with default being the old behavior. Then users who want can switch to the new behavior, and it isn't breaking anyone.

[lyrandy]

Thank you for the great feedback Felix! I've taken in your suggestions.

[lyrandy]

The tests did not like the line ending with the colon character.

- So, for a VM that's been configured using V(VBoxManage modifyvm "vm01" --groups "/TestGroup/TestGroup2,/TestGroup3"):
  C(TestGroup2) is a child group of C(TestGroup); 
  the VM will be part of C(TestGroup2) and C(TestGroup3).

I will reword it to avoid that stylistic choice.

[felixfontein]

The tests did not like the line ending with the colon character.

- So, for a VM that's been configured using V(VBoxManage modifyvm "vm01" --groups "/TestGroup/TestGroup2,/TestGroup3"):
  C(TestGroup2) is a child group of C(TestGroup); 
  the VM will be part of C(TestGroup2) and C(TestGroup3).

That's basic YAML parsing which doesn't like it. A colon followed by a space indicates a dictionary entry in YAML, so you end up with a one-element dictionary with a funny key and value instead of having a string. Simply quoting the thing (or using things like >- etc.) prevents that.

[lyrandy]

The tests did not like the line ending with the colon character.

- So, for a VM that's been configured using V(VBoxManage modifyvm "vm01" --groups "/TestGroup/TestGroup2,/TestGroup3"):
  C(TestGroup2) is a child group of C(TestGroup); 
  the VM will be part of C(TestGroup2) and C(TestGroup3).

That's basic YAML parsing which doesn't like it. A colon followed by a space indicates a dictionary entry in YAML, so you end up with a one-element dictionary with a funny key and value instead of having a string. Simply quoting the thing (or using things like >- etc.) prevents that.

Your explanation makes it so obvious. Thanks!

[felixfontein]

If nobody objects, I'll merge this on Sunday.

[patchback]

Backport to stable-9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-9/21b16c1c77f732e2c763138ffe03ac80a3897214/pr-8510

Backported as #8621

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@lyrandy thanks for your contribution!

[lyrandy]

@lyrandy thanks for your contribution!

Thank you for your guidance and help!