New Collection review for Hitachi's VSP One Object

[nagbattula09]

I am contacting the team to ask for a review of our newly Red Hat Automation Certified collection, vspone-object-ansible. This collection offers automated solutions for performing specific operations on Hitachi's VSP One Object storage system, and we would appreciate feedback and approval from the Steering Committee.

Ansible Galaxy: https://galaxy.ansible.com/ui/repo/published/hitachivantara/vspone_object/
GitHub: https://github.com/hitachi-vantara/vspone-object-ansible
Issues Tracker: https://github.com/hitachi-vantara/vspone-object-ansible/issues
GitHub Handles: @nagbattula09, @huiwang100
Is the collection part of Automation Hub: Yes
We meet Collection Requirements: Yes

[Andersson007]

@nagbattula09 hello, thanks for the submission!
Once someone from @ansible-collections/steering-committee has time, they'll review the request. Thanks

[nagbattula09]

Hi @Andersson007 , Appreciate the update! Looking forward to the review from the steering committee. Thank you.

[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
• follows licensing rules
• 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
• 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)
  MUST FIX: the requirements state Collections MUST support all eligible controller Python versions in the other environment, unless required libraries do not support these Python versions.. The oldest supported version of core is 2.17 and its Python versions range starts with 3.7, but the collection supports >= 3.9 and I don't see any libraries in its requirements that require 3.9. Please make the collection support Python 3.7. If I miss anything, please clarify.
• 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.
  MUST FIX: I see two files that aren't necessary for the collection to function: messages.properties and version.txt - please remove. Also does the tools/ directory contain a file that the users need?

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
  MUST FIX: Please add stable-2.19 to your test matrix. And you can remove stable-2.16 as it's EOL
• 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]

@nagbattula09 I've finished the first review ^. Please take a look at items marked with MUST FIX. Thanks

[nagbattula09]

Hi @Andersson007 , thank you for the review.

We have referenced messages.properties in the code so we would need it. The file under tools/ is for collecting logs so we would need that too.

We will fix the other MUST FIX issues and reply back to the same thread. Thank you.

[nagbattula09]

Hi @Andersson007,

We have addressed all the issues highlighted in your review. Please have a look and review the changes and approve. Also, please let us know if anything else is required to change.

MUST FIX: the requirements state Collections MUST support all eligible controller Python versions in the other environment, unless required libraries do not support these Python versions.. The oldest supported version of core is 2.17 and its Python versions range starts with 3.7, but the collection supports >= 3.9 and I don't see any libraries in its requirements that require 3.9. Please make the collection support Python 3.7. If I miss anything, please clarify.

Fix provided. Changed the documentation of all modules to support 3.7

MUST FIX: I see two files that aren't necessary for the collection to function: messages.properties and version.txt - please remove. Also does the tools/ directory contain a file that the users need?

As discussed before, we have referenced messages.properties in the code so we would need it. The file under tools/ is for collecting logs so we would need that too.

MUST FIX: Please add stable-2.19 to your test matrix. And you can remove stable-2.16 as it's EOL

Fix provided. Added stable-2.19 in .github/workflows/ansible-test.yml

[Andersson007]

@nagbattula09 hi, thanks for the fixes and clarifications! LGTM now

[Andersson007]

@ansible-collections/steering-committee : Second short review is needed here. i've conducted the first inclusion review and approved the collection.
Could anybody please do the second one by checking only the following points (or more if you wish):

• 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

@ansible-collections/steering-committee please conduct the review ^ and explicitly approve the collection.