Include vmware.vmware collection into Ansible package

[machacekondra]

Two or three sentence description of your collection.

Ansible Galaxy: https://galaxy.ansible.com/ui/repo/published/vmware/vmware/
GitHub: https://github.com/ansible-collections/vmware.vmware
Issues Tracker: https://github.com/ansible-collections/vmware.vmware/issues
GitHub Handles: @machacekondra @mikemorency @bardielle @mariolenz
Is the collection part of Automation Hub: Yes
We meet [Collection Requirements](https://docs.ansible.com/ansible/devel/community/collection_contributors/collection_requirements.

[Andersson007]

@ansible-collections/steering-committee i've just reviewed one collection, could anybody please review this one?

[Andersson007]

Ansible Collections Checklist (short version)

For details about the following points, refer to the Collection Requirements.

Every comment should say whether the reviewer expects it to be addressed, or whether it's optional.

Note for reviewers: If you don't know how to check any of the points below, please ask maintainers of the collection you're reviewing or a Steering Committee member for clarifications in comments of corresponding inclusion discussion.

Public availability and communication:

• published on Ansible Galaxy with version 1.0.0 or later
• have a Code of Conduct (CoC) compatible with the Ansible Code of Conduct (CoC)
• have a publicly available issue tracker that does not require a paid level of service to create an account or view issues
• have a public git repository
• releases of the collection are tagged in its repository

Standards and documentation:

• adheres to semantic versioning
  MUST FIX the collection doesn't follow SemVer, for example, adding new modules is not a major change https://github.com/ansible-collections/vmware.vmware/blob/main/CHANGELOG.rst. Please add a link to SemVer to your README and follow it.
• follows licensing rules
• follows the Ansible documentation standards and the style guide
  SHOULD: it's probably a bit late but consider deprecating and removing aliases for username and password in the next major release. It's really a thing we should avoid (official recommendation, not my preference)
  SHOULD: be merged Doc improvements vmware.vmware#21
• follows development conventions; as well as these other requirements:
  • modules satisfy the concept of idempotency
  • modules that only gather information are named <something>_info
    MUST: vm_list_group_by_clusters.py to be renamed to vm_list_group_by_clusters_info.py or to whatever else containing _info.py
  • modules that return ansible_facts are named <something>_facts and do not return non-facts
  • other modules must not allow querying information using specific state option values, or similar mechanisms (like state=get or state=query). These features should be moved to <something>_info or <something>_fact modules.
  • check_mode is supported in all _info and _facts modules
• supports all Python versions supported by all ansible-core versions its supports. If it does not, read the full guidelines to see if you qualify for an exception and document the unsupported Python versions in the collection README.md and in every module and plugin (or in doc fragments)
• only uses the allowed plugin types in the plugins/ directory
• has README.md
• FQCNs are used for all plugins and modules including ansible.builtin. for builtin ones from ansible-core in all their appearances in the documentation, examples, and return sections, and in extends_documentation_fragment:s.
• documentation and return sections use version_added: containing the collection version for which an option, module or plugin was added (except cases when they were added in the very first release of the collection)
• public plugins, roles and playbooks do not use files outside of meta/, plugins/, roles/, and playbooks/

Collection management:

• galaxy.yml having tags field set
• collection dependencies must have a lower bound on the version which is at least 1.0.0, and are all part of the ansible package
• meta/runtime.yml defines the minimal version of Ansible which the collection works with
• has changelog, preferably with changelogs/changelog.yaml
• collection repository should not contain any large objects (binaries) comparatively to the current Galaxy tarball size limit of 20 MB. For example, package installers for testing purposes shouldn't be added.
• collection repository should not contain any unnecessary files like, for example, temporary files. Temporary files should be added to .gitignore.

Tests:

Note for reviewers: If you don't know how to check the points below, please ask maintainers of the collection you're reviewing how you can do it.

• In most cases, CI is set up via GitHub Actions.
• Check .yml files in the .github/workflows directory. There must be at least the sanity section under jobs containing the ansible-test sanity command running against all supported ansible-core versions that must be also listed there, for example, - stable-2.15, - stable-2.16, - stable-2.17.
• Check workflow runs by clicking the Actions tab in the repository's page - you're interested in Scheduled runs, runs against release commits and runs against pull requests.
• If there are no workflows in the Actions tab, ask the collection maintainers how CI is implemented.

• passed ansible-test sanity
• if test/sanity/ignore*.txt exists, it MUST not contain error codes listed in the list of errors that must not be ignored
• has CI tests up and running against each of the "major versions" of ansible-base/ansible-core that the collection supports
• all CI tests MUST run against every pull request
• all CI tests MUST run regularly (nightly, or at least once per week)
• sanity tests MUST run against a commit that releases the collection; if they don't pass, the collection won't be released

[Andersson007]

@machacekondra @mariolenz thanks for submitting the collection! Generally a great work!

I've finished the first round of review, please take a look at MUSTs and SHOULDs:)

[machacekondra]

Thank you very much for your review! We will fix it ASAP!

[bardielle]

@Andersson007 Thank you for your feedback and your time
I published and merge new PR including the name changing you mentioned
Do you think we can publish a new release 2.0.0 (since renaming module is a major change) ?

[Andersson007]

@bardielle thanks for the fast feedback!
Yes, we can release after it's merged (i've just put a couple of comments)
I would also consider removing the extra aliases as written (preferably in a separate PR), though it's not necessary.
Also pleas see the semver related comment above

[bardielle]

@Andersson007 we released the minor version 1.2.0 with all the required changes
Is there anything else we should do now?

[Andersson007]

@machacekondra it's OK now.
One thing to mention is be careful with versioning ansible-collections/vmware.vmware#19 (comment) in future

+1 from me to include this collection to the package, could anyone from @ansible-collections/steering-committee conduct the second review to check only the following items:

• follows the Ansible documentation standards and the style guide
• follows development conventions; as well as these other requirements:
  • modules satisfy the concept of idempotency
  • modules that only gather information are named <something>_info
  • modules that return ansible_facts are named <something>_facts and do not return non-facts
  • other modules must not allow querying information using specific state option values, or similar mechanisms (like state=get or state=query). These features should be moved to <something>_info or <something>_fact modules.
  • check_mode is supported in all _info and _facts modules

[bardielle]

Thank you for your comment, I opened PR for that

[mariolenz]

@machacekondra @bardielle Please have a look at ansible-collections/vmware.vmware#35. @felixfontein pointed out to me that this is just a copy of https://github.com/ansible-collections/vmware.vmware/blob/main/docs/docsite/links.yml and needs to be updated.

[felixfontein]

I didn't mention that in my review since there's already a PR for that :)

[bardielle]

Thank you @mariolenz your PR was merged.

[felixfontein]

I did a second review for this collection. I found two issues that need fixing, and I noted a few suggestions that can be improved (but don't have to for inclusion) below.

• [Needs fixing] The rename of vm_list_group_by_clusters to vm_list_group_by_clusters_info introduced a breaking change: the return value's name of that module changed from vm_list_group_by_clusters to vm_list_group_by_clusters_info. At least in case the module is called with the old name (IIRC you can use not module._name.endswith('_info') to test for that), you have to return the old return value as well (next to the new name).
• [Needs fixing] The modules do not declare their requirements (requirements key in DOCUMENTATION). Just specifying them in the README isn't enough, since the README won't be visible on the Ansible docsite.
• [Suggestion] It would make sense to move the common part of the attributes to a docs fragment (the description for check_mode).
• [Suggestion] The return value documentation for _info modules is somewhat sparse. It's mainly documented by example. I would use contains to at least better document the top-level keys inside the return dictionary.
• [Suggestion] You should include a EE configuration (meta/execution-environment.yml) that explicitly references requirements.txt as the Python requirements file (explicit > implicit).
• [Comment] Not a comment on the collection, but nothing screams "beware of Java developers" as much as seeing Python imports such as from com.vmware.vapi.std_client import DynamicID :)

[felixfontein]

Both PRs seem to fix the issues.

shouldn't we move this from secod review neded to second review in progress?

Yes. I have no idea who should adjust the category (the reviewer?), in any case I've adjusted it now.

[felixfontein]

Once there's a new release with the fixes included, I think this should be OK for inclusion.

[mariolenz]

I have no idea who should adjust the category (the reviewer?), in any case I've adjusted it now.

I didn't find any documentation about this, but I should say that whoever starts the second review should change the category. Maybe we should document this somewhere. That is, if we haven't yet and I'm just too stupid to find it.

Once there's a new release with the fixes included, I think this should be OK for inclusion.

Great! I'll try to find the time to work on your suggestions to improve the collection.

[bardielle]

I'm happy to announce that I just released a new version
When will be the next community package release?

[mariolenz]

When will be the next community package release?

I think 2024-07-16.