New collection review: purestorage.fusion

[sdodsley]

This collection provides support for the Pure Storage Fusion control plane.

Ansible Galaxy: https://galaxy.ansible.com/purestorage/fusion
GitHub: https://github.com/Pure-Storage-Ansible/Fusion-Collection
Issues Tracker: https://github.com/Pure-Storage-Ansible/Fusion-Collection/issues
Is the collection part of Automation Hub: Yes
We meet Collection Requirements: Yes

[Andersson007]

@sdodsley hello, thank you for the submission! I hope the @ansible-collections/steering-committee will take a look ASAP
Here's the Collection inclusion checklist.

[mariolenz]

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.

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 SCM 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

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

  comment: I'm not sure if this is a good requirement any more. The collection runs on the controller, not on the managed node. If we add it to Ansible 5+ (ansible-core 2.12+), the minimum Python version for the control node is 3.8.

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

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

[felixfontein]

• 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)
  comment: I'm not sure if this is a good requirement any more. The collection runs on the controller, not on the managed node. If we add it to Ansible 5+ (ansible-core 2.12+), the minimum Python version for the control node is 3.8.

I don't see why the collection must be run on the controller. It can also be run on another host (there might be some company policy that all access to the storage has to be made from a certain machine, for example). So while the common case is to execute these modules on the controller, they can also be executed somewhere else.

The individual modules already document that they require Pyhton >= 3.5 (with the docs fragment), so I guess the README of the collection should also say that.

[sdodsley]

Added the Python requirement to the README

[mariolenz]

@Andersson007 I've copied the checklist and started to work on it. But I won't promise anything, haven't done this before.

[mariolenz]

@sdodsley It looks like version 1.0.1 is available on galaxy, but not tagged in the repository. I think this is a violation of

have a public SCM repository, releases of the collection are tagged in this repository

[sdodsley]

Fixed

[mariolenz]

@sdodsley

collection repository should not contain any unnecessary files like, for example, temporary files created by development tools. Temporary files MUST be added to .gitignore.

Pure-Storage-Ansible/Fusion-Collection#25

[sdodsley]

Resolved

[mariolenz]

@sdodsley I've found two issues with the examples in the module documentation:

1. Please use the FQCN in fusion_info.py (purestorage.fusion.fusion_info instead of fusion_info)
2. There's a typo in the second example of fusion_ss.py, purestorage.fusion.fusion_s instead of purestorage.fusion.fusion_ss (missing 's')

[sdodsley]

@mariolenz Both fixed

[mariolenz]

@Andersson007 This is my first new collection review, but I should say purestorage.fusion satisfies all the requirements. I hope I didn't blunder.

[felixfontein]

One thing I noticed when glancing at the collection is that it has ANSIBLE_METADATA in the modules. That entry is only needed for Ansible 2.9 (or before) when having non-default values, i.e. for "Supported" modules. We recommend to remove that, even though right now the presence of that field doesn't hurt.

Another quick note: in https://github.com/Pure-Storage-Ansible/Fusion-Collection/blob/master/plugins/modules/fusion_api_client.py#L41 C(...) should be used instead of I(...), and other places such as https://github.com/Pure-Storage-Ansible/Fusion-Collection/blob/master/plugins/modules/fusion_ss.py#L42 use backticks instead of the allowed formatting identifiers (see https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#linking-and-other-format-macros-within-module-documentation).

Also the README says Ansible >= 2.11, which should say ansible-core >= 2.11.

And a final note for today: the copyright header in modules says # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt), while the actual file is called COPYING.GPLv3. So the copyright header should say # GNU General Public License v3.0+ (see COPYING.GPLv3 or https://www.gnu.org/licenses/gpl-3.0.txt).

[sdodsley]

@felixfontein I beleive I have addressed all your concerns.

[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 SCM repository, releases of the collection are tagged in this repository

Standards and documentation:

• adheres to semantic versioning
• follows licensing rules
  COMMENT: I see 2 license files COPYING.GPLv3 and LICENSE, should one of them be removed?
• follows the Ansible documentation standards and the style guide
  MUST FIX: There's no info if modules support check mode or not. Usually there should be something like - Supports C(check_mode). in the Notes: section.
  MUST FIX: In option descriptions in the DOCUMENTATION block some lines end with periods and some not. Please put periods where missed (See the doc standards).
  MUST FIX: If not provided, defaults to C(name) - we use I() for option names. Please recheck if you use proper markdown. Same for cases like To clear the username/password pair use I(clear) as the password as we use C() for values.
  MUST FIX - name: collect default set of information - Examples descriptions must start with a capital letter.
• 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
  MUST FIX: There are no entries for versions v1.0.1 and v1.0.2 in the changelog. Do you use antsibull-changelog?
• 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.
  MUST FIX: There's the runtime.yml file in the repo's rood directory, which, i think, can be removed.

Tests:

• 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: In GH Actions tab i see that sanity tests run regularly but in jobs related to PRs i see only Lint tests ran. Same for release commits. Also see ssbarnea's comment.
• 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
  MUST FIX: See my comment above

[Andersson007]

@sdodsley while i review the collection, could you please (and other maintainers) subscribe to news-for-maintainers if you're still not?

[ssbarnea]

I did a very brief look at the repo and I was surprised to see that on its "lint" job it was only running black. While seeing use of black is good, I would have expected ansible-lint, especially as that is part of upload to galaxy.

[Andersson007]

@sdodsley i finished my review. Please take a look at it and @ssbarnea 's comment.

[sdodsley]

@Andersson007 The CI sanity tests run against every PR, including releases. We run two separate jobs - one for ansible sanity and one for Lint for every PR. So I'm not sure what you are referring to in your MUST FIX for Tests.
All documentation standards have now been adhered to as required.
Extra LICENSE file removed as well as unnecessary runtime.yaml
Release 1.0.1 and 1.0.2 did use antsibull-changlelog but they were just review changes, so nothing in the actual code.
@ssbarnea Ansible-lint added to CI as requested.

[Andersson007]

@sdodsley thanks for clarifying and fixing the stuff, +1 from me to its inclusion.
I've created a corresponding community topic ansible-community/community-topics#117.
If there are no objections from the Steering Committee, I'll add it to the build file on July 7, 2022 and it'll appear in the package in the next Ansible 6.X release.

[Andersson007]

Included ansible-community/ansible-build-data#141, will appear in the next Ansible 6.X release. Thanks everyone!