New collection review: community.sap

[rainerleber]

Collection to provide more functionality to provision and manage SAP Systems with Ansible.

Ansible Galaxy: https://galaxy.ansible.com/community/sap
GitHub: https://github.com/ansible-collections/community.sap
Issues Tracker: https://github.com/ansible-collections/community.sap/issues
GitHub Handles:
Is the collection part of Automation Hub: No
We meet Collection Requirements: Yes

[tadeboro]

Thank you for your submission, @rainerleber.

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
  MUST FIX: The only release available on Ansible Galaxy is 0.1.0.
• has a policy of releasing, versioning and deprecation announced to contributors and users in some way
  MUST FIX: I did not find any information about releases in the repo. Some examples from existing collections: sops, sensu_go
• 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)
• 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
• 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)

[tadeboro]

I finished my first review, and I am happy to report that the collection is almost ready to be included in the Ansible package. I marked the things that we need to fix in the checklist above.

While reviewing the collection, I noticed that the SAP collection does not have any integration tests, yet it is still running them in the CI. So maybe we should disable them in the CI configuration file until some tests are actually in place to be more environment-friendly?

[Andersson007]

@rainerleber thanks for following the collection requirements and submitting the collection almost ready for inclusion!:)
@tadeboro thanks for reviewing!
I'll try to make time to do the same this week

[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.

Public availability and communication:

• published on Ansible Galaxy with version 1.0.0 or later
  MUST FIX: The only release available on Ansible Galaxy is 0.1.0.
• has a policy of releasing, versioning and deprecation announced to contributors and users in some way
  MUST FIX: I found Releasing, Versioning and Deprecation community.sap#4 but please mention that the collection follows semver. Also master in the description should be changed to main.
• 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
  MUST FIX: See the PR Prepare the collection for inclusion community.sap#14. Though not all things are necessary to fix.
• 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: I don't see info about supported Python version in README. Also please update the included content section.
• 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
  MUST FIX: see the PR Prepare the collection for inclusion community.sap#14
• 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
  MUST FIX: see the PR Prepare the collection for inclusion community.sap#14
• 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
• 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)

[Andersson007]

@rainerleber I've done the first round. Could you please take a look. First of all, please take a look at this PR ansible-collections/community.sap#14 (don't release 1.0.0 before merging the PR)

[rainerleber]

@tadeboro @Andersson007 thank for review i will take care of the missing things and the pr

[rainerleber]

@tadeboro @Andersson007 the requested changes are integrated. What are the next steps ?

[Andersson007]

@rainerleber thank you!
If the collection supports all Python versions as Ansible supports (i.e. it's a default), LGTM for inclusion.

[rainerleber]

@Andersson007 yes all Python versions are supported it's also documented here: https://github.com/ansible-collections/community.sap#tested-with-ansible-and-the-following-python-versions

[Andersson007]

@rainerleber great, thanks for clarifying! I'll try to arrange a vote on the inclusion ASAP

[felixfontein]

I glanced over the collection and found a few small issues that should be fixed (that don't really block inclusion though):

1. The URL in https://github.com/ansible-collections/community.sap/blob/main/galaxy.yml#L15 does not exist
2. You want to add flatmap: true to https://github.com/ansible-collections/community.sap/blob/main/changelogs/config.yaml (as in https://github.com/ansible-collections/community.general/blob/main/changelogs/config.yaml#L7)
3. https://github.com/ansible-collections/community.sap/blob/main/plugins/modules/system/sap_system_facts.py#L37-L38 should be replaced by type: dict. (Also the comment above ansible_facts can probably be removed :) )

[rainerleber]

@felixfontein thank you for the suggestions i implemented them.

[Andersson007]

@rainerleber we expect the collection to get included in ~2 weeks provided there are no major concerns appear ansible-community/community-topics#74

[Andersson007]

Included in the next minor release of Ansible 5.
Thanks @rainerleber for the collection!
Thanks @tadeboro and @felixfontein for reviwing!

[rainerleber]

@Andersson007 @felixfontein thank you for help, input and integration :-)

[rainerleber]

@Andersson007 in agreement with the SAP RedHat BU i migrated the community.sap collection to a new place (https://github.com/sap-linuxlab/community.sap_libs) to provide a more konsistent overview of the sap related collections.
Is it possible to change the inclusion from community.sap to https://github.com/sap-linuxlab/community.sap_libs ? It's the same collection with also the other related stuff like versioning,releasing etc. only the Name is changed. Because the next ansible will be released soon it would be greate to change it before the release.

[felixfontein]

@rainerleber I created a discussion topic for it: ansible-community/community-topics#98

Please note that we now also add new collections in minor releases, so when community.sap_libs misses Ansible 6.0.0 it can still be included in 6.1.0, or 6.2.0, or ...

[Andersson007]

@felixfontein thanks for creating the topic!