New collection review t_systems_mms.icinga_director

[rndmh3ro]

• link to the collection on galaxy: https://galaxy.ansible.com/t_systems_mms/icinga_director
• ink to the source code repository: https://github.com/T-Systems-MMS/ansible-collection-icinga-director
• link to the issue tracker: https://github.com/T-Systems-MMS/ansible-collection-icinga-director/issues
• GitHub IDs of collection owners: @rndmh3ro, @schurzi

I think we follow all collection requirements, however we currently have our own changelog (automated).

[dmsimard]

Hi @rndmh3ro && @schurzi and thanks for your interest !

The community has agreed on a list of criteria that must be met in order to move forward with the inclusion of a collection in the Ansible package. For the purpose of this discussion, we will use a shortened checklist to verify and record compliance but you can find the full details in the Collection Requirements documentation.

This is a new and gradual process and we will edit this comment as we progress along -- feel free to ask any questions and provide feedback. Thanks in advance for your understanding !

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

Standards and documentation:

• adheres to semantic versioning
• follows licensing rules
• follows the Ansible documentation standards and the style guide
• follows development conventions
• 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)

[rndmh3ro]

Thanks for reviewing @dmsimard!
I checked the open points:

follows development conventions

I reread these and did:

• add docstrings to all functions: https://github.com/T-Systems-MMS/ansible-collection-icinga-director/pull/81
• added a name-alias for object_name: https://github.com/T-Systems-MMS/ansible-collection-icinga-director/pull/80

modules (or plugins) from ansible-base use ansible.builtin. as a FQCN prefix

Is there anything we need to to here? I'm not aware we use modules from ansible.

documentation and return sections use version_added: containing the collection version for which an option, module or plugin was added

I added them retroactively here: https://github.com/T-Systems-MMS/ansible-collection-icinga-director/pull/82

all CI tests run against a commit that releases the collection; if they don't pass, the collection won't be released

Does this mean we need to create a separate commit for a new release? Currently our releases are based on the last commit made in the main-branch.

[dmsimard]

Hi @rndmh3ro and thanks for following up. The unchecked boxes weren't necessarily unmet criteria but things I didn't get the chance to check and complete yet :)

follows development conventions

I reread these and did:

* add docstrings to all functions: [T-Systems-MMS/ansible-collection-icinga-director#81](https://github.com/T-Systems-MMS/ansible-collection-icinga-director/pull/81)
* added a `name`-alias for `object_name`: [T-Systems-MMS/ansible-collection-icinga-director#80](https://github.com/T-Systems-MMS/ansible-collection-icinga-director/pull/80)

Looks good! Will review and let you know if there are any other improvement opportunities here.

modules (or plugins) from ansible-base use ansible.builtin. as a FQCN prefix

Is there anything we need to to here? I'm not aware we use modules from ansible.

I don't think so! I actually started looking at this one and it doesn't seem applicable here.

documentation and return sections use version_added: containing the collection version for which an option, module or plugin was added

I added them retroactively here: T-Systems-MMS/ansible-collection-icinga-director#82

That's great, thanks !

all CI tests run against a commit that releases the collection; if they don't pass, the collection won't be released

Does this mean we need to create a separate commit for a new release? Currently our releases are based on the last commit made in the main-branch.

This means that tests must run on the git commit (or tag) that end up being released as to prevent releasing the collection in a broken state. For example, 1.9.1 was tagged on top of T-Systems-MMS/ansible-collection-icinga-director@bc6c732 and we can tell that the tests ran for it:

By the way, it's not a blocker but I noticed that the version in galaxy.yml is out of date: https://github.com/T-Systems-MMS/ansible-collection-icinga-director/blob/master/galaxy.yml#L3
Maybe the expectation is that it would be updated automatically by this github action but perhaps it doesn't work: https://github.com/T-Systems-MMS/ansible-collection-icinga-director/blob/master/.github/workflows/galaxy.yml

[rndmh3ro]

Thanks for the explanations!

Maybe the expectation is that it would be updated automatically by this github action but perhaps it doesn't work: https://github.com/T-Systems-MMS/ansible-collection-icinga-director/blob/master/.github/workflows/galaxy.yml

Yeah, I know. :/ That's broken. I have to fix this.

[rndmh3ro]

Yeah, I know. :/ That's broken. I have to fix this.

Fixed this: https://github.com/T-Systems-MMS/ansible-collection-icinga-director/runs/1682548333?check_suite_focus=true (see "update galaxy.ml task")
The correc tversion is now included: https://github.com/T-Systems-MMS/ansible-collection-icinga-director/blob/master/galaxy.yml

[felixfontein]

I looked through some aspects of the collection, I think it looks pretty good! There are some details I recommend improving though:

• In the modules, you base your module's argument spec on a subset of url_argument_spec(), and you do not extend the ansible.builtin.url docs fragment. This causes undocumented options with Ansible's devel branch (use_gssapi option added for ansible-core 2.11). If you only want to use a subset of url_argument_spec() and document the options yourself, you should define the arugment spec yourself as well to make sure it matches your documentation. Or, alternatively, stop using a subset and use both the complete url_argument_spec() and Ansible's url docs fragment.
• GitHub Actions, specifically https://github.com/T-Systems-MMS/ansible-collection-icinga-director/blob/master/.github/workflows/main.yml:
  • The following issues probably are because your file is based on an older version of the collection template (https://github.com/ansible-collections/collection_template/blob/main/.github/workflows/ansible-test.yml), which had some issues.
  • You should never specify the Python version on a ansible-test sanity --docker call, except if you really know what you are doing. If you do that and do not run it for all Python versions supported by ansible-test, you are skipping sanity tests! (Also, you are spending too much time on CI since you are running some tests multiple times without any need for that.)
  • The comments # blocked by ansible/ansible#70155 makes no sense. Remove the comment and both the exclude.
  • The Python version used to install Ansible in Set up Python ${{ matrix.ansible }} does not need to be related to the Python version used to run the tests in the docker container. GitHub Actions can more effectively cache steps if you always use the same version there (for example 3.8).
• Options documentation: you sometimes do not use correct English sentences in module option documentations, for example arguments of the command (should be Arguments of the command.) for the arguments option of the icinga_command module. This is not an explicit requirement, but definitely improves the quality of the documentation. (See https://docs.ansible.com/ansible/devel/dev_guide/developing_modules_documenting.html#documentation-block for an overview.)
• Modules do not seem to document return values at all. (See https://docs.ansible.com/ansible/devel/dev_guide/developing_modules_documenting.html#return-block for how this should be done; if you decide to do that, please do not use type: complex though.)
• In plugins/module_utils/icinga.py, you should use class Icinga2APIObject(object): and not class Icinga2APIObject: for Python 2 compatibility.
• Also running sanity tests against Ansible's devel branch is a good idea, so you're aware earlier about new tests and potential changes in devel that affect your collection.

[rndmh3ro]

As you saw, we fixed these! :)

Any more things we should look at?

[Andersson007]

I had a quick look
About the documentation, i found points below typical for most the modules:

typos: agains, commmand (both several times)
opt zone: explicit "require: false" declaration
opt imports: line 1 does not end with a dot
opt timeout: abbreviations/latin phrases found 'e.g', use 'for example' 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
opt notes_url: has URLs used without U() marker

the ANSIBLE_METADATA block is no longer needed

unnecessary double quotes, e.g.:
35 - "Add or remove a host template to Icinga2 through the director API."

you could use https://github.com/Andersson007/ansible_reviewer to quickly find places to fix

[rndmh3ro]

Thanks, @Andersson007! I used your tool and fixed most of these already here: https://github.com/T-Systems-MMS/ansible-collection-icinga-director/pull/94

I'll have to remove the metadata and check the quotes.

[rndmh3ro]

I'll have to remove the metadata and check the quotes.

Fixed: https://github.com/T-Systems-MMS/ansible-collection-icinga-director/pull/94

[Andersson007]

@rndmh3ro the PR looks good, WY and thanks:)

[dmsimard]

I believe all points have been addressed and have proposed a PR to include t_systems_mms.icinga_director for Ansible 3.0: ansible-community/ansible-build-data#51

[felixfontein]

@rndmh3ro @schurzi congratulations, the PR has just been merged!

[rndmh3ro]

Thank you for this! Like I said in the other issue: that was a really smooth process and nice working with you!