New collection review: community.dns

[felixfontein]

The community.dns collection provides tools for working with DNS:

1. It has a couple of filters for extracting/removing public suffices, and extracting/removing registrable domains from DNS names;
2. It has a module for waiting for TXT entries to propagate to all name servers;
3. It has some modules for working with HostTech DNS entries, and a framework for creating similar modules for DNS provides whose APIs work with individual records (and not record sets).

Ansible Galaxy: https://galaxy.ansible.com/community/dns
GitHub: https://github.com/ansible-collections/community.dns
Issues Tracker: https://github.com/ansible-collections/community.dns/issues
GitHub Handles: @felixfontein
Is the collection part of Automation Hub: No

I think we meet the Collection Requirements.

Note: the collection contains the Public Suffix List, which is licensed under Mozilla Public License (everything else in the collection is GPL v3+). It does not not use Exhibit B of the MPL, which prevents usage with other licenses such as GPL, so I think it should be OK to include this in Ansible (but IANAL).

[tadeboro]

Thank you for your submission, @felixfontein.

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 and patience.

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
• 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: As far as I can tell, the section 3.3 in the license of the Public Suffix List makes this compatible with GPLv3. But as the original submitter, I am also not a lawyer.
• 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 initial review. The community.dns collection is almost ready to become part of the ansible package, but I did manage to find a few spots in the docs where things are a bit lacking.

[felixfontein]

@tadeboro thanks a lot for your review!

* [ ]  follows the [Ansible documentation standards](https://docs.ansible.com/ansible/devel/dev_guide/developing_modules_documenting.html) and the [style guide](https://docs.ansible.com/ansible/devel/dev_guide/style_guide/index.html#style-guide)
  **MUST FIX:** https://github.com/ansible-collections/community.dns/blob/main/plugins/doc_fragments/hetzner.py#L46 is missing `type` (and probably also `elements`) documentation. The same holds for https://github.com/ansible-collections/community.dns/blob/main/plugins/doc_fragments/hetzner.py#L94, https://github.com/ansible-collections/community.dns/blob/main/plugins/doc_fragments/hosttech.py#L59, https://github.com/ansible-collections/community.dns/blob/main/plugins/doc_fragments/hosttech.py#L107

This is intentional (but I guess should be documented better), as these are overrides for https://github.com/ansible-collections/community.dns/blob/main/plugins/doc_fragments/module_record_sets.py#L67 and https://github.com/ansible-collections/community.dns/blob/main/plugins/doc_fragments/inventory_records.py#L37. As the record type depends on the provider, I want to use the doc fragment override mechanism to make the provider doc fragments provide the choices, while the rest is provided by the original docs fragment. Unfortunately it's not that easy, since suboptions can only be replaced completely. That's why the fragments you pointed out basically only define suboptions, but no description, type or anything else.

I'll add a comment each describing this in more detail.

[felixfontein]

I added the details here: ansible-collections/community.dns@a5adbdd

[tadeboro]

I was wondering why the sanity check did not complain, but I was too tired to investigate further, and thus I did not see the duplicated options at all. I am not sure how I feel about this implementation (it is uncomfortably close to the hash_behavior=merge thingy). Maybe some real Python code would be better suited here? Since document fragments are actually imported and not just extracted from the AST, we could probably get rid of the duplication we currently have. But this is a bit out of scope for the review process since the current implementation does work.

[felixfontein]

I've added automation in ansible-collections/community.dns#40 and ansible-collections/community.dns#41, i.e. a script which updates these docs fragments and a sanity test which complains if they weren't updated by the script. I don't think it is a good idea to do magic in docs fragments, I'd consider that they are imported and not extracted from the AST an implementation detail that could change.

[Andersson007]

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
  SUGGESTION: create a pinned release issue aka "Release plan" where maintainers can announce and discuss release related topics.
• 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)

[Andersson007]

@felixfontein could you please take a look at the following import warnings:

/home/aklychko/ansible_collections/community/dns/plugins/module_utils/module/record_info.py:73:13: F841 local variable 'needs_zone_name' is assigned to but never used

/home/aklychko/ansible_collections/community/dns/plugins/module_utils/hetzner/api.py:11:1: F401 'ansible.module_utils.six.moves.urllib.parse.urlencode' imported but unused
/home/aklychko/ansible_collections/community/dns/plugins/module_utils/hetzner/api.py:13:1: F401 'ansible.module_utils.common.text.converters.to_native' imported but unused
/home/aklychko/ansible_collections/community/dns/plugins/module_utils/hetzner/api.py:14:1: F401 'ansible.module_utils.urls.fetch_url' imported but unused
/home/aklychko/ansible_collections/community/dns/plugins/module_utils/hetzner/api.py:32:1: F401 'ansible_collections.community.dns.plugins.module_utils.zone.DNSZoneWithRecords' imported but unused
/home/aklychko/ansible_collections/community/dns/plugins/module_utils/hetzner/api.py:37:1: F401 'ansible_collections.community.dns.plugins.module_utils.zone_record_api.DNSAPIAuthenticationError' imported but unused

/home/aklychko/ansible_collections/community/dns/plugins/module_utils/module/zone_info.py:18:1: F401 'ansible_collections.community.dns.plugins.module_utils.record.format_records_for_output' imported but unused

/home/aklychko/ansible_collections/community/dns/plugins/module_utils/module/record_sets.py:28:1: F401 'ansible_collections.community.dns.plugins.module_utils.zone_record_api.NOT_PROVIDED' imported but unused
/home/aklychko/ansible_collections/community/dns/plugins/module_utils/module/record_sets.py:28:1: F401 'ansible_collections.community.dns.plugins.module_utils.zone_record_api.filter_records' imported but unused

/home/aklychko/ansible_collections/community/dns/plugins/module_utils/json_api_helper.py:16:1: F401 'ansible.module_utils.urls.fetch_url' imported but unused

/home/aklychko/ansible_collections/community/dns/plugins/module_utils/wsdl.py:11:1: F401 'ansible.module_utils.urls.ConnectionError' imported but unused
/home/aklychko/ansible_collections/community/dns/plugins/module_utils/wsdl.py:11:1: F401 'ansible.module_utils.urls.open_url' imported but unused
/home/aklychko/ansible_collections/community/dns/plugins/module_utils/wsdl.py:11:1: F401 'ansible.module_utils.urls.urllib_error' imported but unused
/home/aklychko/ansible_collections/community/dns/plugins/module_utils/wsdl.py:11:1: F401 'ansible.module_utils.urls.NoSSLError' imported but unused

/home/aklychko/ansible_collections/community/dns/plugins/inventory/hosttech_dns_records.py:34:1: F401 'ansible_collections.community.dns.plugins.module_utils.hosttech.api.create_hosttech_argument_spec' imported but unused

/home/aklychko/ansible_collections/community/dns/plugins/inventory/hetzner_dns_records.py:34:1: F401 'ansible_collections.community.dns.plugins.module_utils.hetzner.api.create_hetzner_argument_spec' imported but unused

/home/aklychko/ansible_collections/community/dns/plugins/plugin_utils/inventory/records.py:46:9: F841 local variable 'config' is assigned to but never used

[felixfontein]

Fixed in ansible-collections/community.dns#45.

[Andersson007]

LGTM, thanks for fixing!

[Andersson007]

@felixfontein thanks for the submission! I think some (or all) of the imports mentioned above can be added intentionally. Generally the collection's state LGTM for inclusion.

[Andersson007]

fyi: it was solved

[Andersson007]

+1 for inclusion

[felixfontein]

This collection will be included in Ansible 5: ansible-community/ansible-build-data#70

[felixfontein]

@tadeboro @Andersson007 thanks a lot for your reviews! Also everyone else who took a look, thanks a lot!