New Collection review for Hitachi's VSP One Storage

[rsahuHitachi]

I am contacting the team to ask for a review of our newly Red Hat Automation Certified collection, vspone-block-ansible. This collection offers automated solutions for performing all storage operations, and we would appreciate feedback and approval from the Steering Committee.

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

[rsahuHitachi]

Hi, @ ansible-collections/steering-committee is there any way to make the review process faster?

[rahdey]

One of my colleagues (@rsahuHitachi) submitted the above request for posting Hitachi's Ansible collection hitachivantara.vspone_block's documentation on the docs.ansible.com.

Our collection is already certified by Red Hat
https://console.redhat.com/ansible/automation-hub/repo/published/hitachivantara/vspone_block/?version=3.2.3

@gundalow @Andersson007 @felixfontein could you please review our submission, and let us know if any other steps
are required from Hitachi's side?

We would appreciate, if you could please expedite the process for publishing our documentation on the community site.

Thanks,
Rahul Dey

[Andersson007]

@rsahuHitachi @rahdey hello, thanks for submitting the collection for inclusion

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)
  • MUST FIX: no code of conduct, you can add one and just refer to the Ansible Code of Conduct (CoC) from it.
• 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
  • MUST FIX: releases on Galaxy and tags in the repo do NOT match at all

Standards and documentation:

• adheres to semantic versioning
• follows licensing rules
• follows the Ansible documentation standards and the style guide
  • MUST FIX: Format macros aren't used, i.e. Choices are 'present', 'absent'. should be Choices are C(present), C(absent')., please re-check all. UPDATE: I took a look and found the foolowing. I can't go through all the docs unfortunately, could someone from your team do it and align it with the doc standards like the format macros and other?
    • plugins/modules/vsp/hv_paritygroup.py: the 'update' should also be C(update) in the state argument docs
    • plugins/modules/vsp/hv_hg.py: "set state to I(present)" should be "set state C(present)"
  • MUST FIX: Please specify check mode support in the docs for each module, please use the attributes: field
  • MUST FIX: In EXAMPLES of some modules tasks don't contain - name: ..., e.g. they contain just - hv_cmd_dev: instead
  • MUST FIX: Add the requirements: field to your modules (or a doc_fragment and include it in modules that share the requirement).
  • MUST FIX: Recheck the docs please, i see examples containing FQCNs like hitachivantara.vspone_block.vsp.hitachivantara.vspone_block.vsp.hv_resource_group_lock:
  • MUST FIX: Sentences in option descriptions must start with a capital letter and end with a period, see plugins/modules/vsp/hv_resource_group_lock.py
• 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
    • MUST FIX _facts modules in the collection don't return facts as part of ansible_facts field
  • 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.
  • MUST FIX: they are not used. UPDATE: it's a must for all modules, not only builtin. So please apply this to all modules names in EXAMPLEs
• 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
  • MUST FIX: even if it's technically present, it doesn't match the releases on Galaxy
  • MUST FIX: the link to the changelog from README results in 404
• 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 a Dockerfile and messages.properties file

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

MUST FIX All the below. You can use workflows from https://github.com/ansible-collections/collection_template to set it all up.

• 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]

@rsahuHitachi @rahdey I've finished the first round of review.

• For the workflows and CoC you could use https://github.com/ansible-collections/collection_template.
• For releasing, including changelog generation, you can use this guide https://docs.ansible.com/ansible/devel/community/collection_contributors/collection_release_without_branches.html

If you have any questions along the way, just ask.

Let us know when you fix all the above. Thanks

[rsahuHitachi]

Thanks for your review, we will fix and get back to you with the new version

[rsahuHitachi]

I have one doubt regarding check mode declaratrion , I found this way to declare it , let me know if this is correct one
DOCUMENTATION = """
............

supports_check_mode: true
attributes:
  check_mode:
    description: >
      In check mode, this module simulates changes without modifying the storage system.
    type: bool
    returned: always

[russoz]

Hi @rsahuHitachi no, that is not the correct way.

See one example at:
https://github.com/ansible-collections/community.general/blob/main/plugins/modules/keycloak_user.py#L216-L218

Also check the documentation for valid keywords (not a direct link, click on this and scroll up, the last item from the previous section is attributes):
https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#linking-within-module-documentation

[rsahuHitachi]

Hi Russoz, Thanks for the reply

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.
MUST FIX: they are not used
I did not get it here

even I tried to use
extends_documentation_fragment:

• community.general.attributes

But ending up with an error like
ansible.errors.AnsibleError: unknown doc_fragment(s) in file plugins/modules/vsp/hv_cmd_dev.py: community.general.attributes

what would be the fix here

[rsahuHitachi]

Hi Team,

I have pushed all the fixes to GitHub and Galaxy.

Comments:

collection repository should not contain any unnecessary files like, for example, temporary files. Temporary files should be added to .gitignore.
MUST FIX I see a Dockerfile and messages.properties file
Rabi: We have removed the Dockerfile, but we are using messages.properties in our code, so keeping the file.

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.
MUST FIX: they are not used
Rabi: We are not using any built-in plugin in our module, so skipped this part

[Andersson007]

Hi @rsahuHitachi i've just took another look and there's a significant improvement, thanks for doing this!
There's still a few must-fixes in the list from my review comment above I've just updated. Could you please take another look? Thanks

[rsahuHitachi]

Hi @Andersson007, Thanks for the quick review. I have published a new version with all the "MUST FIX" items fixed. Please review and approve, and let us know if any changes are required.

[rsahuHitachi]

Hi @Andersson007 can you please help us to review the latest changes? Thanks in advance

[Andersson007]

@rsahuHitachi hi, thanks for pinging, I'll do it when I can, probably on Thursday, March 13

[Andersson007]

@rsahuHitachi , I've taken another look

• When i point something out, it doesn't mean it's present only in one file: i can't go through all of them, i pick up a few randomly each time assuming that the issue found should be checked and fixed everywhere.
• I'm afraid there are still doc issues, please take a look at must fixes in the review comment.
• One of the changelog-related must-fixes is still relevant
• About ansible_facts, i see you changed docs stating that they return ansible_facts, but I see, for example, in plugins/modules/vsp/hv_snapshot_group_facts.py that the module just unpacks the following dics to the exit_json() method. Where's the ansible_facts field in there as specified in the ansible documentation?

261         data = {
262             "snapshots": snapshot_data,
263         }

[rsahuHitachi]

Hi @Andersson007, I will look into those and will fix it asap
For changelog fixes, I just followed some other collections like AWS , Cisco's repo, and done the changed accordingly , Do you have any sample changelog file which I can refer ?

[Andersson007]

Hi @rsahuHitachi

• When releasing, i'd recommend to follow the process described in https://docs.ansible.com/ansible/devel/community/collection_contributors/collection_release_without_branches.html
• I also noticed that the changelog doesn't really follow SemVer, e.g. you add new modules under major_changes, but just new not-breaking functionality (like new modules) goes under minor changes. If you release something breaking or potentially breaking for your users (which we generally recommend to avoid) you go with major_changes and do a major release (X.y.z). Look also at https://docs.ansible.com/ansible/devel/community/collection_contributors/collection_releasing.html

[Andersson007]

• as a specific example done using the above guides see https://github.com/ansible-collections/community.mysql/blob/main/CHANGELOG.rst, and all matches with what is on Galaxy

[rsahuHitachi]

hi, @Andersson007 @russoz we have addressed and fixed all the MUST FIX items apart from the below.

collection repository should not contain any unnecessary files like, for example, temporary files. Temporary files should be added to .gitignore.
MUST FIX I see a Dockerfile and messages.properties file
Rabi: We have removed the Dockerfile, but we are using messages.properties in our code, so keeping the file.

Please have a look and review the changes and approve, Please let us know if anything else is required to change.

[Andersson007]

@rsahuHitachi hi, the item you mentioned was marked as resolved before i wrote #79 (reply in thread)
Have you fixed the stuff mentioned ^ and all other must-fixes with checkboxes not marked as resolved?

[rsahuHitachi]

Yes, I have FIXED all the MUST FIX Items
I was confused about the changelog file and was generating it manually, But now I have generated it by using antsibull-changelog module which is fine now. Thanks for the information.

[Andersson007]

@rsahuHitachi approved. Please keep the versions on Galaxy and in the repo matching in future and the changes in the changelog satisfying SemVer. I'll change the category to the Second review needed.

[Andersson007]

@ansible-collections/steering-committee i 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 ^

[rsahuHitachi]

Hey @Andersson007 , I don't see our collection in the latest version index, which was supposed to be present after the monthly release 11.5.0.

[felixfontein]

Do you mean https://docs.ansible.com/ansible/latest/collections/hitachivantara/vspone_block/ ?

[rsahuHitachi]

Oh yes, I was not able to find it. Thanks.

[rsahuHitachi]

Hi Team, We will have a release every month. Do you know if we need to request inclusion for each release, or will it update automatically?

[Andersson007]

@rsahuHitachi hi, nope, you don't need to request it, it'll happen automatically