Include dellemc.powerflex collection

[khareRajshree]

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

Collection Link: https://galaxy.ansible.com/dellemc/powerflex
Repo: https://github.com/dell/ansible-powerflex

[tadeboro]

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

[khareRajshree]

Thanks @tadeboro for providing this information, we will go through the check list and will get back to you.

[tadeboro]

Thank you for your submission, @khareRajshree.

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

Standards and documentation:

• adheres to semantic versioning
• follows licensing rules
  MUST FIX: https://github.com/dell/ansible-powerflex/blob/main/plugins/doc_fragments/dellemc_powerflex.py is missing a license header.
  MUST FIX: galaxy.yml contains both license and license_file options. One of them must go.
• follows the Ansible documentation standards and the style guide
  SHOULD FIX: https://github.com/dell/ansible-powerflex/blob/e494bc2015eab041700a63fb789c534addb46881/plugins/doc_fragments/dellemc_powerflex.py#L47 is not needed since boolean arguments only take true or false values.
  SHOULD FIX: Metadata is ignored by ansible-base and ansible-core and can be removed.
  MUST FIX: Modules do not document the PyPowerFlex python requirement. The readme does contain this information, but reference docs do not.
• 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-powerflex/blob/main/plugins/modules/dellemc_powerflex_gatherfacts.py is an info module, not a facts module (it does not actually 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-powerflex/blob/main/plugins/modules/dellemc_powerflex_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: Since Ansible 5 will depend on ansible-core-2.12, the collection also needs to be tested against stable-2.12 branch.
• 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.

Other:

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

[tadeboro]

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

[khareRajshree]

Hi @tadeboro , we have updated the collections based on the checklist, I request you to please review it once again.

Ansible Galaxy: https://galaxy.ansible.com/dellemc/powerflex
GitHub: https://github.com/dell/ansible-powerflex
Issues Tracker: https://github.com/dell/ansible-powerflex/issues

Thanks,
Rajshree

[khareRajshree]

Hi @tadeboro, a gentle reminder.

[anupamaloke]

@tadeboro, @felixfontein, appreciate your help and review. Now that ansible community package 5.0.0 is out, we are submitting the ansible-powerflex collection for inclusion in ansible community package 6.0.0. Do we need to create a new request or we can continue this discussion thread itself?

[gundalow]

@tadeboro, @felixfontein, appreciate your help and review. Now that ansible community package 5.0.0 is out, we are submitting the ansible-powerflex collection for inclusion in ansible community package 6.0.0. Do we need to create a new request or we can continue this discussion thread itself?

Continue to use this request.
Inclusion reviews are generally done by members of the Ansible Community in their spare time.

[tadeboro]

Currently, we are still targeting Ansible 6.0.0 with inclusions. But things might change once the voting on ansible-community/community-topics#63 is over. If the proposed idea passes the vote, we will try to get this collection included in the Ansible community package as soon as possible and will not wait for 6.0.0.

As for the silence with reviews: the sad truth is that there is not much happening right now. There are multiple reasons for this (people being busy with other things, etc.), but one of the reasons is also the lack of urgency due to Ansible 6 being almost half a year away. And the linked proposal tries to change this and help speed things up a bit.

[anupamaloke]

@gundalow, @tadeboro, thank you so much for your prompt reponse. I like the new proposal that allows inclusion of the collection in the minor releases as well and voted for it :-)

[anupamaloke]

@felixfontein, @tadeboro, @gundalow, now that voting is completed and ansible inclusion process is simplified, I just wanted to check when we can expect the collection to be included in the ansible community release. I know you guys have been very busy, so there is no rush.

[anupamaloke]

@tadeboro, just checking to make sure that this is also there in your list of collections that need to be included in the next minor release :-)

[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 a lot for your update.

[Andersson007]

@khareRajshree @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 @ansible-collections/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]

@anupamaloke i don't think all the issues have been fixed.. you could start fixing them while I'm reviewing the collection, no need to wait

[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
  MUST FIX: (copied from tadaboro's review) 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
  MUST FIX: Also add info that you follow SemVer.
• have a Code of Conduct (CoC) compatible with the Ansible Code of Conduct (CoC)
  MUST FIX: (copied from tadaboro's review) 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

Standards and documentation:

• adheres to semantic versioning
• follows licensing rules
  MUST FIX: (copied from tadaboro's review) doc_fragments/dellemc_powerflex.py is missing a license header.
  MUST FIX: (copied from tadaboro's review) galaxy.yml contains both license and license_file options. One of them must go.
• follows the Ansible documentation standards and the style guide
  MUST FIX: (copied from tadaboro's review) Modules do not document the PyPowerFlex python requirement. The readme does contain this information, but reference docs do not.
  MUST FIX: Use proper markup (see the doc guidelines "Module format and documentation"

for example, in the following fragment, sds_id must be I(sds_id). I can see many of other doc formatting issues across the files including in doc_fragments/powerflex.py. Please recheck all the files and make the doc satisfying the doc standards:

  38   sds_name:
  39     description:
  40     - The name of the SDS.
  41     - Required while adding a device.
  42     - Mutually exclusive with sds_id.

• 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)
  MUST FIX: As collection does not support all the required Python versions, please add to doc_fragments/powerflex.py requirements: the Python versions the collection supports.
• 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 created by development tools. Temporary files MUST 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
• all CI tests MUST run regularly (nightly, or at least once per week)
  MUST FIX: they do not run regularly, please add scheduled run directive to your GHA matrix
• sanity tests MUST run against a commit that releases the collection; if they don't pass, the collection won't be released

[Andersson007]

@anupamaloke I've done with the first round of my review. Could you please take a look?
Also the issues from it can be relevant to your second inclusion request. If you take them into consideration there, before someone else starts its review, it'll speed up the progress, I'm sure.

thanks

[anupamaloke]

@Andersson007, thank you for reviewing this. I will get started on getting these comments fixed.

[kuttattz]

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

[Andersson007]

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

[Andersson007]

@kuttattz hi, looks almost ready for inclusion, thanks for fixing the stuff!
Two last points from me:

• I created a PR do make the docs adhering to the latest doc standards (a decision has been made recently about the boolean representation), needs to get merged Change doc formatting for inclusion in Ansible dell/ansible-powerflex#21
• I don't see CI runs (sanity + units) against my PR, should be fixed

Please let me know when they are done

[Jennifer-John]

Hi @Andersson007 the CI runs on Change doc formatting for inclusion in Ansible PR is approved to run and they were successful and the PR is merged. Thank you!

[Andersson007]

@Jennifer-John ah, sorry, i see now, thanks!

@felixfontein @mariolenz @gotmax23 Tadej's PR wasn't finished but as far as I can see the maintainers fixed things he spotted. Would you like to take a quick look and confirm this?

[mariolenz]

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.

[felixfontein]

Hmm, asking for specific OS versions without a good reason sounds like a no-go to me. I would guess that these are not hard requirement but more statements on what has been tested, but if that's the case the wording should really be changed.

And yes, ansible-core 2.14 not being mentioned in the README is odd.

[Jennifer-John]

Thank you @mariolenz @felixfontein for the review. Yes the readme will be updated with support for 2.14 as part of the release on 13th dec as we were still working on the regression tests on 2.14. As you have rightly mentioned, these are the tested RHEL versions. We will try to re-word or remove it from the pre-requisites. Once we have the release, I will post here the updated links to the readme.
Thanks,
Jennifer.

[Andersson007]

@Jennifer-John thanks for the fast feedback, please let us know when we can take another look

[Jennifer-John]

sure @Andersson007, thank you!

[Jennifer-John]

Hi @Andersson007 , @felixfontein @mariolenz 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/powerflex.
Could you please help re-review https://github.com/dell/ansible-powerflex 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 I think this collections is fit to be added to the community package.

[Andersson007]

@mariolenz feel free to submit a topic similar to ansible-community/community-topics#163 (or I can after I'm back from PTO)

[mariolenz]

@mariolenz feel free to submit a topic similar to ansible-community/community-topics#163 (or I can after I'm back from PTO)

FYI @Andersson007: ansible-community/community-topics#177

[Jennifer-John]

Thank you @mariolenz for the review, @Andersson007 @felixfontein 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.

[ssbarnea]

-1 CG should not grow, especially with code that is not "general"-enough.

[Andersson007]

@ssbarnea if you think we should formally (update: in a formal way i mean) vote on the inclusion, please put a not in the topic ^

[Jennifer-John]

Thank you @Andersson007 for the update

[mariolenz]

@Jennifer-John I've opened ansible-community/ansible-build-data#193 but it's not merged yet. But I hope someone will do soon.

cc @Andersson007

[Andersson007]

Done, thanks everyone!:)

[Jennifer-John]

Thank you @mariolenz @Andersson007! Thank you team for the reviews!

Thanks,
Jennifer