community.general icon indicating copy to clipboard operation
community.general copied to clipboard

Update VirtualBox Group parsing to align with documentation.

Open lyrandy opened this issue 1 year ago • 5 comments

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"
    ]
}

lyrandy avatar Jun 15 '24 05:06 lyrandy

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

plugins/inventory/virtualbox.py:225:77: W291: trailing whitespace
plugins/inventory/virtualbox.py:244:1: W293: blank line contains whitespace

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

plugins/inventory/virtualbox.py:225:77: W291: trailing whitespace
plugins/inventory/virtualbox.py:244:1: W293: blank line contains whitespace

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

plugins/inventory/virtualbox.py:225:77: W291: trailing whitespace
plugins/inventory/virtualbox.py:244:1: W293: blank line contains whitespace

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

plugins/inventory/virtualbox.py:225:77: W291: trailing whitespace
plugins/inventory/virtualbox.py:244:1: W293: blank line contains whitespace

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

plugins/inventory/virtualbox.py:225:76: trailing-whitespace: Trailing whitespace
plugins/inventory/virtualbox.py:244:0: trailing-whitespace: Trailing whitespace

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

plugins/inventory/virtualbox.py:225:76: trailing-whitespace: Trailing whitespace
plugins/inventory/virtualbox.py:244:0: trailing-whitespace: Trailing whitespace

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

plugins/inventory/virtualbox.py:225:76: trailing-whitespace: Trailing whitespace
plugins/inventory/virtualbox.py:244:0: trailing-whitespace: Trailing whitespace

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

plugins/inventory/virtualbox.py:225:76: trailing-whitespace: Trailing whitespace
plugins/inventory/virtualbox.py:244:0: trailing-whitespace: Trailing whitespace

click here for bot help

ansibullbot avatar Jun 15 '24 05:06 ansibullbot

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.

felixfontein avatar Jun 15 '24 20:06 felixfontein

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

lyrandy avatar Jun 28 '24 03:06 lyrandy

The test botmeta failed with 8 errors:

plugins/inventory/virtualbox.py:0:0: Cannot load DOCUMENTATION: while scanning a simple key
  in "<unicode string>", line 40, column 17::0:
                    C(TestGroup2) is a child group o ... :0:0:
                    ^:0:0:
could not find expected ':':0:
  in "<unicode string>", line 41, column 17::0:
                    the VM will be part of C(TestGro ... :0:0:
                    ^:0:0:

The test extra-docs failed with 8 errors:

plugins/inventory/virtualbox.py:0:0: Did not return correct DOCUMENTATION
plugins/inventory/virtualbox.py:0:0: Missing documentation or could not parse documentation: community.general.virtualbox did not contain a DOCUMENTATION attribute (/tmp/antsibull-docs-8ofe1nj4/ansible_collections/community/general/plugins/inventory/virtualbox.py). Unable to parse documentation in python file '/tmp/antsibull-docs-8ofe1nj4/ansible_collections/community/general/plugins/inventory/virtualbox.py': while scanning a simple key
                                       in "<unicode string>", line 40, column 17:0:0:
                                     could not find expected ':':0:
                                       in "<unicode string>", line 41, column 17. while scanning a simple key:0:0:
                                       in "<unicode string>", line 40, column 17:0:0:
                                     could not find expected ':':0:
                                       in "<unicode string>", line 41, column 17:0:0:

The test ansible-test sanity --test yamllint [explain] failed with 2 errors:

plugins/inventory/virtualbox.py:49:17: error: DOCUMENTATION: syntax error: could not find expected ':' (syntax)
plugins/inventory/virtualbox.py:49:17: unparsable-with-libyaml: while scanning a simple key - could not find expected ':'

The test ansible-test sanity --test yamllint [explain] failed with 2 errors:

plugins/inventory/virtualbox.py:49:17: error: DOCUMENTATION: syntax error: could not find expected ':' (syntax)
plugins/inventory/virtualbox.py:49:17: unparsable-with-libyaml: DOCUMENTATION: while scanning a simple key - could not find expected ':'

The test ansible-test sanity --test yamllint [explain] failed with 2 errors:

plugins/inventory/virtualbox.py:49:17: error: DOCUMENTATION: syntax error: could not find expected ':' (syntax)
plugins/inventory/virtualbox.py:49:17: unparsable-with-libyaml: while scanning a simple key - could not find expected ':'

The test ansible-test sanity --test yamllint [explain] failed with 2 errors:

plugins/inventory/virtualbox.py:49:17: error: DOCUMENTATION: syntax error: could not find expected ':' (syntax)
plugins/inventory/virtualbox.py:49:17: unparsable-with-libyaml: DOCUMENTATION: while scanning a simple key - could not find expected ':'

The test ansible-test sanity --test ansible-doc [explain] failed with the error:

Command "ansible-doc -t inventory community.general.virtualbox" returned exit status 1.
>>> Standard Error
ERROR! inventory community.general.virtualbox missing documentation (or could not parse documentation): community.general.virtualbox did not contain a DOCUMENTATION attribute (/root/ansible_collections/community/general/plugins/inventory/virtualbox.py). Unable to parse documentation in python file '/root/ansible_collections/community/general/plugins/inventory/virtualbox.py': while scanning a simple key
  in "<unicode string>", line 40, column 17
could not find expected ':'
  in "<unicode string>", line 41, column 17. while scanning a simple key
  in "<unicode string>", line 40, column 17
could not find expected ':'
  in "<unicode string>", line 41, column 17

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

plugins/inventory/virtualbox.py:0:0: deprecation-mismatch: "meta/runtime.yml" and DOCUMENTATION.deprecation do not agree.
plugins/inventory/virtualbox.py:0:0: missing-documentation: No DOCUMENTATION provided
plugins/inventory/virtualbox.py:49:17: documentation-syntax-error: DOCUMENTATION is not valid YAML

The test ansible-test sanity --test ansible-doc [explain] failed with the error:

Command "ansible-doc -t inventory community.general.virtualbox" returned exit status 1.
>>> Standard Error
ERROR! inventory community.general.virtualbox missing documentation (or could not parse documentation): community.general.virtualbox did not contain a DOCUMENTATION attribute (/root/ansible_collections/community/general/plugins/inventory/virtualbox.py). Unable to parse documentation in python file '/root/ansible_collections/community/general/plugins/inventory/virtualbox.py': while scanning a simple key
  in "<unicode string>", line 40, column 17
could not find expected ':'
  in "<unicode string>", line 41, column 17. while scanning a simple key
  in "<unicode string>", line 40, column 17
could not find expected ':'
  in "<unicode string>", line 41, column 17

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

plugins/inventory/virtualbox.py:0:0: deprecation-mismatch: "meta/runtime.yml" and DOCUMENTATION.deprecation do not agree.
plugins/inventory/virtualbox.py:0:0: missing-documentation: No DOCUMENTATION provided
plugins/inventory/virtualbox.py:49:17: documentation-syntax-error: DOCUMENTATION is not valid YAML

The test ansible-test sanity --test ansible-doc [explain] failed with the error:

Command "ansible-doc -t inventory community.general.virtualbox" returned exit status 1.
>>> Standard Error
ERROR! inventory community.general.virtualbox missing documentation (or could not parse documentation): community.general.virtualbox did not contain a DOCUMENTATION attribute (/root/ansible_collections/community/general/plugins/inventory/virtualbox.py). Unable to parse documentation in python file '/root/ansible_collections/community/general/plugins/inventory/virtualbox.py': while scanning a simple key
  in "<unicode string>", line 40, column 17
could not find expected ':'
  in "<unicode string>", line 41, column 17. while scanning a simple key
  in "<unicode string>", line 40, column 17
could not find expected ':'
  in "<unicode string>", line 41, column 17

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

plugins/inventory/virtualbox.py:0:0: deprecation-mismatch: "meta/runtime.yml" and DOCUMENTATION.deprecation do not agree.
plugins/inventory/virtualbox.py:0:0: missing-documentation: No DOCUMENTATION provided
plugins/inventory/virtualbox.py:49:17: documentation-syntax-error: DOCUMENTATION is not valid YAML

The test ansible-test sanity --test ansible-doc [explain] failed with the error:

Command "ansible-doc -t inventory community.general.virtualbox" returned exit status 1.
>>> Standard Error
ERROR! inventory community.general.virtualbox missing documentation (or could not parse documentation): community.general.virtualbox did not contain a DOCUMENTATION attribute (/root/ansible_collections/community/general/plugins/inventory/virtualbox.py). Unable to parse documentation in python file '/root/ansible_collections/community/general/plugins/inventory/virtualbox.py': while scanning a simple key
  in "<unicode string>", line 40, column 17
could not find expected ':'
  in "<unicode string>", line 41, column 17. while scanning a simple key
  in "<unicode string>", line 40, column 17
could not find expected ':'
  in "<unicode string>", line 41, column 17

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

plugins/inventory/virtualbox.py:0:0: deprecation-mismatch: "meta/runtime.yml" and DOCUMENTATION.deprecation do not agree.
plugins/inventory/virtualbox.py:0:0: missing-documentation: No DOCUMENTATION provided
plugins/inventory/virtualbox.py:49:17: documentation-syntax-error: DOCUMENTATION is not valid YAML

click here for bot help

ansibullbot avatar Jun 28 '24 03:06 ansibullbot

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.

lyrandy avatar Jun 28 '24 04:06 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.

felixfontein avatar Jul 05 '24 06:07 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.

Your explanation makes it so obvious. Thanks!

lyrandy avatar Jul 11 '24 03:07 lyrandy

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

felixfontein avatar Jul 12 '24 20:07 felixfontein

Backport to stable-9: 💚 backport PR created

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

Backported as https://github.com/ansible-collections/community.general/pull/8621

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

patchback[bot] avatar Jul 14 '24 10:07 patchback[bot]

@lyrandy thanks for your contribution!

felixfontein avatar Jul 14 '24 10:07 felixfontein

@lyrandy thanks for your contribution!

Thank you for your guidance and help!

lyrandy avatar Jul 15 '24 01:07 lyrandy