New Collection review: netapp.cloudmanager

[carchi8py]

We've created a few new collections for our other products (same team that currently support the existing NetApp collections). We do monthly update to each collection (And bug fixes throughout the month as needed)

• Ansible Galaxy: https://galaxy.ansible.com/netapp/cloudmanager
• Github: https://github.com/ansible-collections/netapp
• Issues Tracker: https://github.com/ansible-collections/netapp/issues
• Github Handles: @carchi8py @lonico

[Andersson007]

Hi @carchi8py ! Thanks for the collection!

I'll put the following check list here and fill it out myself later when reviewing the collection. Thank you!

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 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
  Comment: I didn't find neither an issue nor a wiki page. It MUST be there. See pinned issues in community.general or in community.postgresql for an example.
• have a Code of Conduct (CoC) compatible with the Ansible Code of Conduct (CoC)
  Comment: I don't see it anywhere, MUST be added (or a ref to Ansible CoC in README)
• 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
  Comment: didn't find the information, MUST be there
• follows licensing rules
  Comment: Found GPL v2 in galaxy.yml, MUST be GPL v3 or later https://github.com/ansible-collections/overview/blob/main/collection_requirements.rst#licensing. There's GPL v3 in the modules
• follows the Ansible documentation standards and the style guide
  Comment: The following MUST be fixed:

Remove ANSIBLE_METADATA from all the modules

Typos:
togther
seperated

Common for most the modules (one random example of each issue):
short_description: line 1 ends with a dot
opt provider_volume_type: line 4 does not end with a dot
opt capacity_tier: abbreviations/latin phrases found 'won't', use 'will not' instead
no "notes" section, it should, at least, contain info about check_mode support
examples: "- name" of #1 does not start with a capital letter
examples: #1 no FQCN
return: no RETURN section, there must be, at least, RETURN = r"""#"""
opt workspace_id: has URLs used without U() marker
return msg: line 1 does not end with a dot
return msg: no sample

To quickly check all the modules, you can use https://github.com/Andersson007/ansible_reviewer with the key '-s' to check spelling (there can be false warnings).

• follows development conventions
• only uses allowed plugin types in the plugins/ directory
• has README.md
• documentation, examples, and return use FQCNs for M(..), examples, and seealso subsections
• modules (or plugins) from ansible-base 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 is used for extends_documentation_fragment:, unless you are referring to doc_fragments from ansible-base

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
  Comment: Didn't find
• has changelog, preferably with changelogs/changelog.yaml
  Comment: Didn't find

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)

[carchi8py]

We pull in every request into our internal branch before, and have it pass our pipeline before we approve it on github.

I did forgot to add it to the github action we have setup for all commit that runs ansible-test. I'll go ahead and make that change.
https://github.com/ansible-collections/netapp/blob/master/.github/workflows/main.yml

[felixfontein]

One remark on https://github.com/ansible-collections/netapp/blob/master/.github/workflows/main.yml: you are only running a subset of all sanity tests. Please run ansible sanity --docker=default without --python and without plugins/modules/*. You are skipping several tests for other Python versions (import and compile most notably), and you don't test all non-module files. (There's no need to run sanity tests in a Python version matrix as well, one call without --python runs tests with all relevant ones.)

[carchi8py]

I'l make that change in the future. Currently (i did a test run) if we run with out --python. We hit the following issue
ansible/ansible#74014. Though i check if i can modify the default python on github actions to by a non python2 and that should get around that issue.

We should have the changes for most of the stuff above out later today i'll update this when it get pushed.

[felixfontein]

The issue should have been fixed (i.e. SNI support is back for files.pythonhosted.org). In any case, if you really want to use --python, you have to make sure that you cover all supported Python versions. You are currently skipping 3.8 and 3.9 (3.9 does only work for stable-2.10 and devel).

[carchi8py]

just noticed it was fix on our internal pipeline after i sent that

[Andersson007]

@gundalow @felixfontein this collection is under ansible_collections and it has master branch as its default branch. Must it be renamed first?

[gundalow]

Given there are multiple collections in here then a different directory structure is need, so I'm OK with this as is. Though we shouldn't have this structure for new repos which only have a single collection.

Moving from master to main is suggested via not a hard requirement

[Andersson007]

@carchi8py please remove all ANSIBLE_METADATA from the modules

[carchi8py]

We should have changed with most of the change above getting pushed out to github Monday

[tadeboro]

I only did an initial look into the collection (I only looked at the administrative side of things for now) and the fact that there are several collections hosted in the same repository does not feel right. The main reason for this is release management. I found several git tags in the repository that do not have a corresponding Ansible Galaxy artifact. I guess this happens because a new tag is created when any of the collections need to be released, and once the tagging is done, only a subset of artifacts is created and uploaded to Ansible Galaxy.

Missing documentation on release policy combined with the fact that the collections are not really using semantic versioning makes things even worse. Because the version number is date-based, it is hard to determine how to properly pin the version number in the requirements files. Is it still safe to set the upper bound to the next major version or will some minor version break backward compatibility?

So for starters, the umbrella repository will need to have a pretty detailed document on release policy that describes the release process and what artifacts to expect on Ansible Galaxy, and what backward-compatibility users can expect.

[carchi8py]

We're using the same format that we have used for the last 2 years that was set up with Redhat (@thedoubl3j and a few others).

When we first covered the 2.9 modules to collections we had 4 products with modules we were told to put all the collections in the same repo that RedHat created for us. And so we've built our process around this.

We release a new version on the first Wednesday of each month. 21.3.0 is the same git hash for all collections that had an update (if no code has changed that collection does not get a 21.3.0

On versioning, we talked with RedHat about this 2 years ago and they were ok with YY.MM.Version. We haven't been told in our monthly meeting about with RedHat about any change to this format.

Module are currently tested against Ansible 2.9 and 2.10, and will shortly will be updating our internal pipeline to test against 3.x soon.

[tadeboro]

We're using the same format that we have used for the last 2 years that was set up with Redhat (@thedoubl3j and a few others).

When we first covered the 2.9 modules to collections we had 4 products with modules we were told to put all the collections in the same repo that RedHat created for us. And so we've built our process around this.

As far as I can tell, you are the only vendor right now that has all of their collection hosted in the same repository. And while there is nothing wrong with this approach, it is different from the standard practice (one repo -> one collection) and you will need to at least thoroughly document your development and release processes.

We release a new version on the first Wednesday of each month. 21.3.0 is the same git hash for all collections that had an update (if no code has changed that collection does not get a 21.3.0

On versioning, we talked with RedHat about this 2 years ago and they were ok with YY.MM.Version. We haven't been told in our monthly meeting about with RedHat about any change to this format.

The versioning scheme you are using does indeed match the semantic versioning regex, but semantic versioning is more than just a few numbers separated with dots. The main point of semantic versioning is to set the user expectations around backward-compatibility, availability of bugfixes, etc.

For example, collection users expect that the major version only changes when the release contains (potentially) breaking changes such as removal of deprecated functionality, changes in the existing API, ... Another assumption would be that if I (wearing my Ansible user hat) pin the collection version to something like netapp.cloudmanager:<22.0.0, I would still get the bugfixes for the 21.x.y version.

This last point is especially important for inclusion in the Ansible package because the packages get pinned in a similar way I showed above when a new major version of the Ansible package is prepared. For example, if everything goes according to plan, the community will release Ansible 5.0.0 sometime near the end of 2021. Collections will be pinned at that time to the latest major version available, which in your case will be 21.x.y. If you find a bug in one of your collections in January 2022 and release it as 22.z.u, Ansible users will have to wait almost half a year to get that bugfix if they do not want to manually install the collection from Ansible Galaxy.

This is why it is crucial that you document your release processes and possibly make sure they work with the processes the Ansible community has in place.

Module are currently tested against Ansible 2.9 and 2.10, and will shortly will be updating our internal pipeline to test against 3.x soon.

Testing against 2.10 and 3.0 is exactly the same, so there is no point in doing that. What you should be testing against are different versions of Ansible's core. Right now, this means that you should be testing against ansible==2.9.*, ansible-base, and ansible-core==2.11.0b3 (or the devel branch from the ansible/ansible GitHub repo). That last one is not strictly needed right now but highly recommended.

[carchi8py]

Most of the change request above have been pushed out to github in the following change
ansible-collections/netapp@443ed8d#diff-96304a856b37c1abbc3966eccd246e0e16366f4e085bd2592d472a9eef5e34b8

[tadeboro]

Congratulations! As far as I am concerned, this collection is ready for inclusion in the Ansible community package once a new release with all the changes is created.

[tadeboro]

Thank you for your submission, @carchi8py.

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.

Feel free to ask any questions and provide feedback. Thanks in advance for your understanding.

For details about the following points, refer to the Collection Requirements.

---

Public availability and communication:

• published on Ansible Galaxy 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
  Comment: The current policy is available at Releasing, Versioning and Deprecation netapp#93, but currently, it is hard to find because it is not linked from the readme file.
• 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
  Comment: While the repo is public, the commit history is useless (https://github.com/ansible-collections/netapp/commits/main/ansible_collections/netapp/cloudmanager). It would be much better if the GitHub repo was a direct clone of the Bitbucket repo. This way, potential contributors have at least some idea why certain changes were made. Also, right now, tags on Github and tags on Bitbucket probably point to a different commit, which is strange if my assumption is correct.

Standards and documentation:

• adheres to semantic versioning
  Comment: The repository as a whole now adheres to semantic versioning, but the individual collections do not because they will see version increments of more than 1. I think this is good enough for inclusion in the ansible PyPI package because it still gives all the guarantees to the end-users strict adherence to semver would.
• follows licensing rules
• follows the Ansible documentation standards and the style guide
  Comment: Most modules do not document parameter restrictions that are passed to the module constructor. For example, mutually-exclusive parameters are not documented at all, and some combinations of parameters that seem valid from reading the documentation are ignored by the implementation (https://github.com/ansible-collections/netapp/blob/45a49fa11ea70b7885ae5a1134b56c83ad493c3c/ansible_collections/netapp/cloudmanager/plugins/modules/na_cloudmanager_cifs_server.py#L185-L188 ignores the working_environment_name parameter value if working_environment_id is passed at the same time).
• follows development conventions
• 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 recognized plugin types in the plugins/ directory.
• has README.md
• documentation, examples, and return use FQCNs for M(..), examples, and seealso subsections
• modules (or plugins) from ansible-base 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 is used for extends_documentation_fragment:, unless you are referring to doc_fragments from ansible-base

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

[gundalow]

@carchi8py Great to hear that you've managed to move Semantic versioning. I know that will really help your users.

[tadeboro]

I did another review of the collection and as far as I can tell, there are only three things missing:

1. A few FQCNs in examples.
2. Minor adjustment of CI pipeline that would test against all stable versions of ansible(-base|-core).
3. Nightly CI run.

Once those are fixed, I think this is ready to go.

[lonico]

I'm addressing the FQCN issues (I found 4). @carchi8py do you understand what needs to be done for CI?

[carchi8py]

For number 2. Is there a list somewhere of what the stable version of base and core are?
right now i'm running against 4.0.0a3 (https://github.com/ansible-collections/netapp/blob/main/.github/workflows/main.yml#L22)
and the latest change in devel in the current pipeline (https://github.com/ansible-collections/netapp/blob/main/.github/workflows/main.yml#L35)

[tadeboro]

I created ansible-collections/netapp#100 that should solve CI issues. I left the unit tests out for now in order to get things in a state that is OK for inclusion.

[tadeboro]

Just a reminder: collections that want to be part of the Ansible 4.0.0 package need to get approval by 2021-04-26 (full schedule). This inclusion request still has some open issues that need to get addressed by then.

[carchi8py]

We should have the change request pushed later today (currently under code review in our internal branch). I"ll update this thread when it gets pushed out to github.

[carchi8py]

@tadeboro the new version with the changes above has been pushed out.

[tadeboro]

Last reminder for inclusion submission

Collections that want to be part of the Ansible 4.0.0 package need to get approval by 2021-04-26, which is just one week away (full schedule). Make sure you submit your collection for the final review a few days before the deadline (reviews take a non-trivial amount of time) and that your collection does meet all criteria listed in the requirements document.

[carchi8py]

@tadeboro the new version has been pushed to galaxy, please let us know if there anything else we need to do
https://galaxy.ansible.com/netapp/cloudmanager

[felixfontein]

While looking at this collection, I noticed the following:

• In the docs fragment's notes:, \\_ ends up as \_ in the documentation (see ansible-doc output).
• Also I don't see any module prefixed with cloudmanager_netapp (which I guess cloudmanager\_netapp refers to?), only with na_cloudmanager.

@carchi8py am I misunderstanding that note?

[carchi8py]

@felixfontein sorry could you give me more details on what your asking on this one?
Also I don't see any module prefixed with cloudmanager_netapp (which I guess cloudmanager_netapp refers to?), only with na_cloudmanager.

[felixfontein]

@carchi8py https://github.com/ansible-collections/netapp/blob/main/ansible_collections/netapp/cloudmanager/plugins/doc_fragments/netapp.py#L21 mentions that, and https://github.com/ansible-collections/netapp/tree/main/ansible_collections/netapp/cloudmanager/plugins/modules contains no module with such a prefix.

[carchi8py]

Ahhh thank for that, that a mistake it should be na_cloudmanager

[carchi8py]

I'll get a fix out and a .1 out for that

[carchi8py]

@felixfontein 20.5.1 has been pushed out with this change (and it on galaxy).

[felixfontein]

This collection has been prepared for inclusion in Ansible 4 in this PR: ansible-community/ansible-build-data#66

[tadeboro]

The netapp.cloudmanager Ansible Collection was accepted into the Ansible community package. The relevant part of the community meeting starts here.

Congratulations!