Include dellemc.unity collection

[rajendraindukuri]

Intent is to include dellemc.unity collections in Ansible 5.0.0 community release. Please validate and provide your feedback for the same.

Ansible Galaxy: https://galaxy.ansible.com/dellemc/unity
GitHub: https://github.com/dell/ansible-unity
Issues Tracker: https://github.com/dell/ansible-unity/issues
Is the collection part of Automation hub? : soon for this release (previous releases were part of Automation Hub)

[tadeboro]

@rajendraindukuri Please read the README file and supply all required information. And please do go over the checklist before asking for inclusion review. Thank you.

[rajendraindukuri]

@rajendraindukuri Please read the README file add supply all required information. And please do go over the checklist before asking for inclusion review. Thank you.

Sure @tadeboro . Will go through the checklist and get back if there are any queries

[rajendraindukuri]

Hi @tadeboro ,
Thanks for the references.

Went through the check list and have few queries. Can you please help us with the following queries:

• Our default branch is 'master' in our repo contrary to "All new repositories MUST have main as the default branch" - Is this mandatory?
• Our ChangeLog is .md format and under the root folder of collections contrary to "has changelog, preferably with changelogs/changelog.yaml". Is this fine?
• Is it mandatory to have check_mode for _facts file (all the calls in this file are get calls in our collections) ?
• "has a policy of releasing, versioning and deprecation announced to contributors and users in some way" - Can you please guide us on what exactly needs to be done as part of this?

Thanks

[tadeboro]

• Our default branch is 'master' in our repo contrary to "All new repositories MUST have main as the default branch" - Is this mandatory?

This is only required for repositories hosted in the ansible-collection GitHub organizations. External repositories are free to select the name of the default branch. But switching to the main as default branch would not hurt ;)

• Our ChangeLog is .md format and under the root folder of collections contrary to "has changelog, preferably with changelogs/changelog.yaml". Is this fine?

It is. But as the requirements docs state, if you use an external changelog file, changes from your collection will not be included in the porting guide for Ansible. For example, dellemc.unity collection will not be listed in https://docs.ansible.com/ansible/devel/porting_guides/porting_guide_4.html

• Is it mandatory to have check_mode for _facts file (all the calls in this file are get calls in our collections) ?

All facts and info modules should support check mode. In the vast majority of cases, there is nothing developers need to do apart from declaring support since those modules must not change the state of the target system. In your case, the only module I found that gathers information from the remote target is https://github.com/dell/ansible-unity/blob/master/plugins/modules/dellemc_unity_gatherfacts.py and that module should support chack mode. Also, it would be more appropriate to call that modules something along the lines of dellemc_unity_info, since it does not work as a facts module.

• "has a policy of releasing, versioning and deprecation announced to contributors and users in some way" - Can you please guide us on what exactly needs to be done as part of this?

There are different options on how one can satisfy this condition. For example, the community.general collection uses a pinned issue that contains general information and on each new release, the maintainer adds a new comment. The sensu.sensu_go collection has dedicated documents for release policy and release notes. Network collections use Twitter to inform users about new releases. But what you do does not really matter as long as this is linked in the readme document and updated regularly.

[tadeboro]

Thank you for your submission, @rajendraindukuri.

The items in the checklist can be in one of the following states:

• Items with a checkmark and no attached comment are OK and need no intervention from your side.
• Items with a checkmark that contain a comment will not block the inclusion process, but by following the advice given in the comment, the collection's quality can be improved.
• Items with no checkmark and no extra details are waiting for evaluation.
• Items with no checkmark that contain a comment need your attention.

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.

Public availability and communication:

• published on Ansible Galaxy with version 1.0.0 or later
• has a policy of releasing, versioning and deprecation announced to contributors and users in some way
  MUST FIX: This information is not available in the collection. Some examples: https://sensu.github.io/sensu-go-ansible/release_policy.html, https://github.com/ansible-collections/community.sops#releasing-versioning-and-deprecation
• have a Code of Conduct (CoC) compatible with the Ansible Code of Conduct (CoC)
  MUST FIX: Code of conduct is missing from the collection.
• 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 SCM repository, releases of the collection are tagged in this repository
  SHOULD FIX: Ansible Galaxy is missing a 1.0.0 release.

Standards and documentation:

• adheres to semantic versioning
• follows licensing rules
  MUST FIX: https://github.com/dell/ansible-unity/blob/master/plugins/doc_fragments/dellemc_unity.py is missing a license header.
• follows the Ansible documentation standards and the style guide
  SHOULD FIX: https://github.com/dell/ansible-unity/blob/71528d74aba389a60191a81a161e942c3c9b98c7/plugins/doc_fragments/dellemc_unity.py#L47 is not needed since boolean arguments can only have true or false value.
  SHOULD FIX: https://github.com/dell/ansible-unity/blob/71528d74aba389a60191a81a161e942c3c9b98c7/plugins/modules/dellemc_unity_host.py#L11-L13 is not needed since ansible-base and ansible-core ignore this information.
  MUST FIX: Modules do not document storops Python requirement.
• follows development conventions; as well as these other requirements:
  • 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: https://github.com/dell/ansible-unity/blob/master/plugins/modules/dellemc_unity_gatherfacts.py is an info module since it does not set 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.
    MUST FIX: As far as I can tell, all modules are a mix of info and regular modules (when the state argument is set to present, a certain combination of parameters makes modules behave as info modules). This functionality should not be part of the normal module.
  • check_mode is supported in all _info and _facts modules
    MUST FIX: https://github.com/dell/ansible-unity/blob/master/plugins/modules/dellemc_unity_gatherfacts.py does not support check mode.
• supports Python 2.6 or greater and Python 3.5 or greater. 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
• documentation, examples, and return sections use FQCNs for the M(..) format macros when referring to modules
• modules (or plugins) from ansible-core use ansible.builtin. as a FQCN prefix
• documentation and return sections use version_added: containing the collection version for which an option, module or plugin was added
• FQCNs are used for extends_documentation_fragment:, unless you are referring to doc_fragments from ansible-core

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

Tests:

• passed ansible-test sanity
• if test/sanity/ignore*.txt exists, it does not contain error codes listed here
• has CI tests up and running against each of the "major versions" of ansible-base/ansible-core that the collection supports
  MUST FIX: CI is missing tests against stable-2.11 and stable-2.12 ansible branches.
• all CI tests run against every pull request
• all CI tests run against a commit that releases the collection; if they don't pass, the collection won't be released
• all CI tests run regularly (nightly, or at least once per week)
  MUST FIX: There is no scheduled CI run defined in the repo.

Other:

• MUST FIX: galaxy.yml file points to tags in the repository, documentation, and homepage sections.

[tadeboro]

I finished my initial review and the collection is not ready for inclusion in the current state.

[rajendraindukuri]

Thanks @tadeboro . We will work on these action items and get back to you.

[rajendraindukuri]

Hi @tadeboro ,

Please help us understand the following queries:

MUST FIX: Modules do not document storops Python requirement. - Does this mean we need to mention python requirements for storops? If yes , storops SDK do not mention anything about a specific python version as prerequisite - https://github.com/emc-openstack/storops. Please let us know if the intention is something else.

MUST FIX: As far as I can tell, all modules are a mix of info and regular modules (when the state argument is set to present, a certain combination of parameters makes modules behave as info modules). This functionality should not be part of the normal module.

• The intent of this approach is to have basic info about all components in gather_facts (which we will rename to info module now) and use that info to query a specific object or do operations like modify, delete using that info (like id, name etc) from component specific modules. And this decision is made after getting a nod from some of the customers. Please let us know your thoughts on this.

Thanks
Rajendra

[rajendraindukuri]

@tadeboro One more question reg comment MUST FIX: CI is missing tests against stable-2.11 and stable-2.12 ansible branches.

As of now we are claiming support only for ansible 2.9 and 2.10, that the reason we did not add. Is this mandatory?

[felixfontein]

@rajendraindukuri since you applied for your collection to be included in Ansible 5, which is based on ansible-core 2.12, we do expect that you actually support that version.

[tadeboro]

@rajendraindukuri Also, your meta file (https://github.com/dell/ansible-unity/blob/master/meta/runtime.yml#L2) states that you support all Ansible versions newer than 2.9,

[rajendraindukuri]

Hi @tadeboro @felixfontein Updated the repo https://github.com/dell/ansible-unity and incorporated the following feedback given by you

This is only required for repositories hosted in the ansible-collection GitHub organizations. External repositories are free to select the name of the default branch. But switching to the main as default branch would not hurt ;) - Done

All facts and info modules should support check mode. In the vast majority of cases, there is nothing developers need to do apart from declaring support since those modules must not change the state of the target system. In your case, the only module I found that gathers information from the remote target is https://github.com/dell/ansible-unity/blob/master/plugins/modules/dellemc_unity_gatherfacts.py and that module should support chack mode. Also, it would be more appropriate to call that modules something along the lines of dellemc_unity_info, since it does not work as a facts module. - Done

MUST FIX: Code of conduct is missing from the collection. - Done

MUST FIX: https://github.com/dell/ansible-unity/blob/master/plugins/doc_fragments/dellemc_unity.py is missing a license header. - Done

SHOULD FIX: https://github.com/dell/ansible-unity/blob/71528d74aba389a60191a81a161e942c3c9b98c7/plugins/doc_fragments/dellemc_unity.py#L47 is not needed since boolean arguments can only have true or false value. - Done

SHOULD FIX: https://github.com/dell/ansible-unity/blob/71528d74aba389a60191a81a161e942c3c9b98c7/plugins/modules/dellemc_unity_host.py#L11-L13 is not needed since ansible-base and ansible-core ignore this information. - Done

MUST FIX: Modules do not document storops Python requirement. - This is a dependency that we are using.. can you help understand why and where we should mention the python version for this?

MUST FIX: https://github.com/dell/ansible-unity/blob/master/plugins/modules/dellemc_unity_gatherfacts.py is an info module since it does not set facts. - Done

MUST FIX: As far as I can tell, all modules are a mix of info and regular modules (when the state argument is set to present, a certain combination of parameters makes modules behave as info modules). This functionality should not be part of the normal module. - Done

MUST FIX: https://github.com/dell/ansible-unity/blob/master/plugins/modules/dellemc_unity_gatherfacts.py does not support check mode. - Done

Can you please take a look?

Thanks
Rajendra

[anupamaloke]

@tadeboro , just checking to make sure that this is there in your list of collections for inclusion for next minor relase whenever that is :-)

[tadeboro]

I am not part of the Ansible community anymore, so you will need to find someone else to push this forward.

[anupamaloke]

@tadeboro, thanks for your update.

[Andersson007]

@rajendraindukuri @anupamaloke have all the issues from tadeboro's review been fixed?

If yes, I'll change the category to the Second review needed and will announce via Bullhorn that the collection needs another review.
As soon as there's a volunteer from the Community or https://github.com/orgs/ansible-collections/teams/steering-committee , after the collection is reviewed the second time, it'll be included after that.

[anupamaloke]

@Andersson007, thank you for your response. Yes, we have fixed all the issues from tadebro's review.

[Andersson007]

@rajendraindukuri @anupamaloke maybe you could forward to Dell that several other dellemc collections are on the verge of exclusion from the package, see issue 1, issue 2, issue 3, just a fyi.
Here's the Removal process for details.

[anupamaloke]

@Andersson007 , thank you for bringing it to my notice. I will ask the respective maintainers of these collections to take actions.

[Andersson007]

@anupamaloke thanks! They can ping me or @mariolenz in corresponding collection repositories (there are also issues about removal, the latest ones) if they have any questions about how they should proceed to stay in the package.

[mariolenz]

@Andersson007 Not to put too fine a point on this, but I don't think that asking the respective "maintainers" of dellemc.os6 / 9 / 10 to take actions is really a solution.

If they don't react on the issues I've opened they look unmaintained to me. If @anupamaloke has to make them aware of this, it's even another sign that those collections are not really maintained.

The basic problem still exists: There hasn't been any activity from the maintainers on those collections during the last 6+ months.

[Andersson007]

@mariolenz i totally agree with you, I'm trying to avoid extra work from our side in the future to get the collection back if all the issues are solved and collection get regularly maintained again. Of course, we'll proceed with the exclusion now and will not stop it now even if someone appears shortly promising to fix stuff.
We'll be monitoring the situation up to Ansible 8 release.

[felixfontein]

I've glanced a bit into the collection, here are some comments:

• The meta/runtime.yml redirects are invalid, they do not use FQCN: https://github.com/dell/ansible-unity/blob/main/meta/runtime.yml#L6 This is exposed by the current sanity tests in ansible-core devel.
• Also CI isn't run against stable-2.14.
• Non-standad names are used for options, like https://github.com/dell/ansible-unity/blob/main/plugins/doc_fragments/unity.py#L29 is usually called validate_certs. Better rename the option (and keep the old name as an alias, and eventually deprecate and remove it).
• There are no Ansible versions 2.11, 2.12, 2.13: https://github.com/dell/ansible-unity/blob/main/plugins/doc_fragments/unity.py#L47 (that should be ansible-core, not Ansible)
• The collection claims to support Ansible 2.9.1 and newer (meta/runtime.yml), but only tests against 2.11+, and https://github.com/dell/ansible-unity/blob/main/plugins/doc_fragments/unity.py#L47 mentiones 2.11, 2.12, and 2.13
• Documentation is not formatted properly, for example names of options as in https://github.com/dell/ansible-unity/blob/main/plugins/modules/tree_quota.py#L32 are not in I(...). See https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#linking-and-other-format-macros-within-module-documentation
• In tree_quota, one return value is documented as complex (which shouldn't be used, use type: dict or type: list to be more helpful for users), but a string is returned here: https://github.com/dell/ansible-unity/blob/main/plugins/modules/tree_quota.py#L661
• The error message for missing requirements should be composed with missing_required_lib (https://github.com/dell/ansible-unity/blob/main/plugins/modules/tree_quota.py#L283).

[anupamaloke]

@felixfontein, thank you for your feedback.
@previnkumar_g, FYA,

[felixfontein]

Thanks, the issues I found are noew resolved.

[Previnkumar-G]

Hi @felixfontein, @Andersson007 , We have incorporated the latest feedbacks suggested and updated the repo - https://github.com/dell/ansible-unity. Requesting you to review it and let us know if the code is compliant now. Thanks

[Andersson007]

@Previnkumar-G thanks for pinging, I'll try to take a look as soon as I have time

[Andersson007]

@Previnkumar-G thanks for fixing the stuff spotted by reviewers!
A couple of points from me:

• I don't see sanity and units run against Fix doc formatting for inclusion in Ansible dell/ansible-unity#27 UPDATED: I see it now
• Fix doc formatting for inclusion in Ansible dell/ansible-unity#27 should be merged

Looking forward to your feedback, thanks

[Jennifer-John]

Hi @Andersson007 the https://github.com/dell/ansible-unity/pull/27 PR is now merged. Thank you!

[Andersson007]

@Jennifer-John cool, thanks!
@felixfontein could you please take a look at the current collection's state to check if the point you wrote in your review are fixed? (they are as far as i can see)

[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
• has a policy of releasing, versioning and deprecation announced to contributors and users in some way
• 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 this repository

Standards and documentation:

• adheres to semantic versioning
• follows licensing rules
• follows the Ansible documentation standards and the style guide
  MUST FIX Fix doc formatting for inclusion in Ansible dell/ansible-unity#27 must be merged
• follows development conventions; as well as these other requirements:
  • 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 Python 2.6 or greater and Python 3.5 or greater. 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
• documentation, examples, and return sections use FQCNs for the M(..) format macros when referring to modules
• modules (or plugins) from ansible-core use ansible.builtin. as a FQCN prefix
• documentation and return sections use version_added: containing the collection version for which an option, module or plugin was added
• FQCNs are used for extends_documentation_fragment:, unless you are referring to doc_fragments from ansible-core
• 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.11, - stable-2.12, - stable-2.13.
• 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 here
• 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
  MUST FIX: I don't see them running Fix doc formatting for inclusion in Ansible dell/ansible-unity#27 UPDATED: I see it now
• 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]

+1 for inclusion. I think i can consider @felixfontein 's comment as approval as well:) If it wasn't, please let me know.

@Jennifer-John could you please release the collection so that the changes made appear on Galaxy?

It'll get included in 2022-12-06 provided that the collection is released before that date. A topic has been created.

[Jennifer-John]

Hi @Andersson007 , we are currently working on the 1.5.0 release version for dellemc.unity collection which includes the community inclusion changes. The release plan date for the updated collection code on github and galaxy is on 13-12-2022. Can the updated 1.5.0 collection be considered for the inclusion?
Thank you!

[Andersson007]

@Jennifer-John hi, sure, i'll change the date in the issue for 2022-12-16 then to avoid preliminary inclusion, thanks for the info!

[Andersson007]

Also fyi: it will appear in the package in the next Ansible minor release after that.
Also i'll probably be on PTO after 16.12, so would be nice if release 1.5.0 won't be delayed so that I can include it on 16.12.
Please put a comment here when the release happens.

[Jennifer-John]

sure, thank you @Andersson007 !

[Andersson007]

FYI I'm having my PTO since Monday 2022-12-12, I'll ask someone else to proceed with inclusion after you release the collection. Max I'll include it myself in the first week of 2023 (if it's released before that).

[gotmax23]

This collection suffers from the same issue that @mariolenz brought up in the dellemc.powerflex (#33 (comment)) review:

Looks mostly OK to me. However, I'm not sure if we should accept specific RHEL versions as prerequisites. I think Python and ansible-core versions are OK, but why a specific distro?

That said, not supporting 2.14 is a problem since Ansible 7 is based on this. It looks like the CI tests agaisnst against 2.14, so it might be just an outdated documentation in the README.

I'm -1 to inclusion until that's resolved.

[Jennifer-John]

Hi @Andersson007 , @felixfontein @mariolenz @gotmax23 Thank you for the feedback. We have updated the readme to remove the specific RHEL versions and 2.14 is now included in supported versions. The latest changes are released and are part of the ansible galaxy release 1.5.0 https://galaxy.ansible.com/dellemc/unity.
Could you please help re-review https://github.com/dell/ansible-unity for inclusion?

Thanks,
Jennifer

[mariolenz]

The README looks OK to me now. Additionally, it looks like they test against ansible-core 2.14.

@Andersson007 @felixfontein @gotmax23 I think this collections is fit to be added to the community package.

[Andersson007]

Feel free to include it then ansible-community/community-topics#163

[Jennifer-John]

Thank you @mariolenz for the review, @Andersson007 @felixfontein @gotmax23 i also want to bring to your notice post the last review apart from the readme changes, there have also been code changes (which is part of the released collection) for which we have incorporated the community guidelines.
Thanks,
Jennifer.

[mariolenz]

Looks like we're agreed to add dellemc.unity. I've created ansible-community/ansible-build-data#192 to do this.

[Andersson007]

@Jennifer-John congratulations with the inclusion! If there hasn't been minor Ansible releases since @mariolenz added it to configs, the collection will appear in the next minor release.

Thanks everyone!

[Jennifer-John]

Thank you @Andersson007 for the update! Thank you team for the reviews.

Thanks,
Jennifer