New collection: paloaltonetworks.panos

[jamesholland-uk]

Ansible collection with modules that automate configuration and operational tasks on Palo Alto Networks' Next Generation Firewalls, both physical and virtualized form factors, using the PAN-OS API.

Ansible Galaxy: https://galaxy.ansible.com/paloaltonetworks/panos
GitHub: https://github.com/PaloAltoNetworks/pan-os-ansible
Issues Tracker: https://github.com/PaloAltoNetworks/pan-os-ansible/issues
GitHub Handles: @jamesholland-uk @shinmog
Is the collection part of Automation Hub: Yes
We meet Collection Requirements: Yes

[Andersson007]

hello @jamesholland-uk , thanks for submitting the collection for review!
We weekly post inclusion requests in Bullhorn calling for help.
The collection will get reviewed as soon as there are volunteers.
Thank you.

[jamesholland-uk]

Thank you :-)

[jamesholland-uk]

Hi @Andersson007, this does not seem to be getting any traction, is there anything we can do?
Thanks, James

[Andersson007]

@jamesholland-uk hi, sorry for the late reply, was on PTO
yeah, i continue posting this but there are no volunteers yet.
i'll try to find time to review the collection myself 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.

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 git 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: please use FQCNs everywhere when referring to modules (e.g. in EXAMPLES, etc.)
  MUST FIX: please make your RETURN sections satisfying https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#return-block (e.g. i see an issue in plugins/modules/panos_address_group.py, please check the rest)
  MUST FIX: Also please carefully recheck style-guide compliance, e.g. I can see name:'s in EXAMPLES ending with dots but they must not, etc.
  MUST FIX: Markup is not used for things like URLs
  COMMENT: you could use the attributes: field for things like check mode support instead of notes:. It looks much better imo
• follows development conventions; as well as these other requirements:
  • modules satisfy the concept of idempotency
  • 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.
    COMMENT TO ANOTHER SC FAMILIAR WITH RESOURCE MODULES: needs your eyes
  • check_mode is supported in all _info and _facts modules
    MUST FIX: please add support (in the one i checked in docs is written that it's not supported but looks like it's actually supported, please make docs and code match)
• 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 (except cases when they were added in the very first release of the collection)
• 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. Temporary files should be added to .gitignore.
  SHOULD FIX: I can see a few hidden files in the repo root. If they are not necessary for the majority of devs, they should be removed (it's "should", not "must")

Tests:

Note for reviewers: If you don't know how to check the points below, please ask maintainers of the collection you're reviewing how you can do it.

• In most cases, CI is set up via GitHub Actions.
• Check .yml files in the .github/workflows directory. There must be at least the sanity section under jobs containing the ansible-test sanity command running against all supported ansible-core versions that must be also listed there, for example, - stable-2.11, - stable-2.12, - stable-2.13.
• Check workflow runs by clicking the Actions tab in the repository's page - you're interested in Scheduled runs, runs against release commits and runs against pull requests.
• If there are no workflows in the Actions tab, ask the collection maintainers how CI is implemented.

• passed ansible-test sanity
• if test/sanity/ignore*.txt exists, it MUST not contain error codes listed in the list of errors that must not be ignored
• has CI tests up and running against each of the "major versions" of ansible-base/ansible-core that the collection supports
  MUST FIX: add testing against relevant ansible-core versions.
  COMMENT: consider dropping support (and resp testing against) the EOL versions for core.
• all CI tests MUST run against every pull request
• all CI tests MUST run regularly (nightly, or at least once per week)
  MUST FIX: I see the scheduled runs directive is commented in the WF file. Please make tests run regularly
• sanity tests MUST run against a commit that releases the collection; if they don't pass, the collection won't be released

[Andersson007]

@jamesholland-uk

• passed ansible-test sanity: i didn't test it now, so don't worry (as well as about other not ticked items w/o SHOULD/MUST FIX). I'll check it after you fix MUST FIXes
• resource modules is an exceptional (and controversial case). If modules are not resource modules at least, the general rules are applied. I'm not a spec in resource modules. Are your modules resource?

[jamesholland-uk]

Thanks @Andersson007.

• passed ansible-test sanity: Thanks, understood
• Are your modules resource?: That's the intention, yes. Reading through their introduction and current state (ref, ref) that was the target of the team here and why they went down the path of deprecating the dedicated _facts modules in favour of single modules where users define the state as present, merged, absent, gathered, as appropriate.

[Andersson007]

@jamesholland-uk OK, thanks for clarifying! I've changed MUST FIX to COMMENT for myself not to forget to ask someone familiar after we finish the first review. Please fix the rest and ping me. Thanks

[felixfontein]

If the collection contains resource modules, at least one of the reviews should be done by someone who is very familiar with resource modules. Resource modules have a very strict specification.

[jamesholland-uk]

Hi @felixfontein, thank you for the insight. Is it possible to have someone familiar take an initial look at the collection as it currently exists? Having made the move away from dedicated _facts modules in favour of using the states (present, gathered, etc) methodology within the modules, which gives significant benefits to users but also us as maintainers, I think we would be highly unlikely to go back now. Therefore the assessment by a subject-matter expert seems to be a go/no-go gate on proceeding any further with this request for inclusion. Thank you

[russoz]

Hi @jamesholland-uk @Andersson007

I did run the sanity tests and I got quite a number of results particularly unused imports from pylint, but also a number of documentation formatting for parameters, deprecation, etc.

Additionally, virtually all files tripped on this one:

ERROR: plugins/modules/panos_vlan.py:0:0: missing-gplv3-license: GPLv3 license header not found in the first 20 lines of the module

Given that the collection is licensed with Apache License, I wonder whether the licensing rules checkbox should be ticked.

And last but not least, @jamesholland-uk mentioned the tests are always executed but, in the Makefile we have:

.PHONY: old-sanity
old-sanity:		## Sanity tests for Ansible v2.9 and Ansible v2.10
	ansible-test sanity -v --skip-test pylint --skip-test rstcheck --python $(python_version)

.PHONY: new-sanity
new-sanity:		## Sanity tests for Ansible v2.11 and above
	ansible-test sanity -v --skip-test pylint --python $(python_version)

which is not how we treat our good friend pylint. If there must be any exceptions to the relentlessness of pylint, they should be made explicit in the code with comments like:

from ansible.module_utils.urls import open_url  # pylint: disable=import-error

But clearly that would not be the preferred way to handle the issues pointed by pylint.

[felixfontein]

At least in CI ansible-test sanity --docker (or something equivalent) should be run. If you want to run it locally without --docker and a specific Python version, feel free to do that, but in CI you should really do not use --python but use --docker (except if you really, really know what you are doing). Otherwise you are potentially going to miss a lot of problems.

[jamesholland-uk]

Thanks for the comments and reviews folks. I see there are a number of items to work through. I will message back on this thread with an update as things progress. Thanks again