New collection review: cisco.ise

[racampos]

Hello, I'd like to submit the cisco.ise collection for review:

Ansible Galaxy: https://galaxy.ansible.com/cisco/ise
Github repo: https://github.com/CiscoISE/ansible-ise
Github handles: @wastorga @racampos @jbogarin

To the best of my knowledge, the collection satisfies all the requirements and is ready for review.

[Andersson007]

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
• 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
  REQUIRED: I see release 1.0.3 as latest on Galaxy but there's no corresponding tag in the repo - the latest is 1.0.2.

Standards and documentation:

• adheres to semantic versioning

• follows licensing rules
  COMMENT: I'm not a lawyer but all containing MIT what I found in Fedora project licence table looks compatible with GPLv3

• follows the Ansible documentation standards and the style guide
  REQUIRED:

  • Grep -r for "check_mode" returned 0. Maybe it's OK for resource modules, I don't know.

  NOTE: HTML markup in the doc needs thoughts from the doc team.

• 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
    REQUIRED:
    • I don't see info about check_mode support anywhere.
      NOTE: As i'm not familiar with resource modules (including its best practices), it requires review by network folks.

• 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)
  NOTE: does not support Python2 because as mentioned in README, SDK doesn't support it.
  REQUIRED: document unsupported Python versions in every module / plugin OR in a doc fragment used everywhere.

• 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
  REQUIRED: the test didn't pass. Please fix the following: RECOMMENDATION: Just take it into consideration (i ran the sanity tests from devel).

1. the pylint issues

ERROR: Found 2 pylint issue(s) which need to be resolved:
ERROR: plugins/module_utils/ise.py:13:4: ansible-bad-module-import: Import external package or ansible.module_utils not ansible.errors
ERROR: plugins/module_utils/personas_utils.py:5:4: ansible-bad-module-import: Import external package or ansible.module_utils not ansible.errors

2. Also i suppose there should be ignore-2.12.txt for testing against devel.

• 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
  RECOMMENDATION: As said in the requirements: You SHOULD suggest to additionally run ansible-test sanity from the ansible/ansible devel branch so that you find out about new linting requirements earlier.
• 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
  COMMENT: where can i check this?
• all CI tests run regularly (nightly, or at least once per week)

[Andersson007]

I can see HTML markup in the doc, for example ./plugins/modules/network_access_conditions.py. cc @samccann , is it allowed?

[Andersson007]

@racampos thanks for the submission! i made a basic review, please see REQUIRED, COMMENT, and RECOMMENDATION things above.

[Andersson007]

Review updated. Should be fine

[Andersson007]

As i'm not familiar with resource modules (including its best practices), it requires review by network folks.
cc @jillr

[tadeboro]

As far as I can tell, the collection does not use resource modules. The maintainers just opted to implement all modules as action plugins. I quickly looked into the implementation. It seems that all plugins advertise support for check mode (they do not override the https://github.com/ansible/ansible/blob/872fda881540172ef94a9e81150b37ec021a6f8e/lib/ansible/plugins/action/__init__.py#L60). Yet, they never check the play context to determine if the plugin should only simulate the changes.

@racampos, can you please elaborate on the check mode support, please? Thank you.

[racampos]

Hello @tadeboro. As far as we understand, check mode is only mandatory for _info and _facts modules/plugins. By not overriding the default setting (self._supports_check_mode = True) we are implicitly supporting it.

Regarding the rest of the plugins (the ones that actually can perform changes) we don't support check mode, so that's why we don't check for it. I guess our mistake was not overriding the default setting to set it to False. Is that right or am I missing something?

[tadeboro]

There are less than two weeks left for collections to get ready for inclusion in Ansible 5 (see roadmap for more details). This candidate still has unresolved issues and is not ready for inclusion right now.

[tadeboro]

Thank you for your submission, @racampos.

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
• 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
  SHOULD FIX: GitHub repository is missing a 1.0.1 tag (Ansible Galaxy has this version listed).

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
  SHOULD FIX: Changelog entries could be structured a bit better. The last three patch releases do not contain any change entries, just release notes.

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)

[wastorga]

Hi @tadeboro. Thanks.

Could you review the following changes?

Now, info modules have blocks like https://github.com/CiscoISE/ansible-ise/blob/develop/plugins/action/aci_bindings_info.py#L87 where we only return the result.
Furthermore, for regular modules, we removed the offensive block https://github.com/CiscoISE/ansible-ise/blob/develop/plugins/action/aci_settings.py#L216.

For info modules, as I understood it, the offensive line was the one that set "changed" to true.

I'm waiting for your response to approve the pull request and upload a new release.

[tadeboro]

Why do you even need to check for check mode in info modules? Properly designed and implemented info modules should be always safe to run since they must not modify the resource they are interacting with. Is this not true in your modules and things like https://github.com/CiscoISE/ansible-ise/blob/aa9adeb9489e28e0a628a5af6d9e03d38ae5c4a2/plugins/action/aci_bindings_info.py#L96-L100 modify the state? Also, info modules should always return changed as false.

The fix for the regular modules is OK.

[wastorga]

There was a misunderstanding on the need to simulate the info modules when run in check mode. Which I now know is not the case, because they are always safe to run.
All the collection info modules only get information from ISE. The code you pointed out handles subsequent calls to handle pagination.

The issue has been addressed now.

[tadeboro]

I finished my last review, and as far as I am concerned, this collection is ready for inclusion in Ansible 5. Thank you, @racampos, for your patience and cooperation!

[racampos]

On the contrary, thank you @tadeboro for your high quality input. BTW, we have this other collection for which we already fixed all the issues, as well. Would appreciate if you guys could take a look at it before the Ansible 5 deadline. The review process has been idle for 12 days now.

[felixfontein]

The cisco.ise collection was approved for inclusion in Ansible 5. Congratulations!

The inclusion PR: ansible-community/ansible-build-data#79

[racampos]

Hello @tadeboro @felixfontein. The version of cisco.ise that currently comes bundled with Ansible is 1.2.1, which was the current version 6 months ago when the collection was approved for inclusion. However, the current version on our Github repo as well as on Ansible Galaxy is 2.3.1. What do we need to do in order to update the version that gets installed along with Ansible?

[felixfontein]

@racampos you have to wait for Ansible 6. Every major version of Ansible avoids breaking changes, and major version bumps in a collection's version indicate breaking changes (semantic versioning). See this section for details: https://github.com/ansible-collections/overview/blob/main/collection_requirements.rst#versioning-and-deprecation

[racampos]

Thanks @felixfontein, for the clarification. What's the ETA for Ansible 6?

[felixfontein]

You can find the roadmap here: https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/roadmap/COLLECTIONS_6.rst#release-schedule

[wastorga]

Hello @felixfontein, @gundalow, @tadeboro.
I would appreciate it if you could respond to some doubts I have.

1. Do we need to create a new discussion for the latest version of the collection?
2. Or can we expect the latest version of cisco.ise before the freeze to be added to Ansible 6.0.0?

[MarkusTeufelberger]

These versions of cisco.ise will make it into Ansible 6.x:

https://github.com/ansible-community/ansible-build-data/blob/698a274b68ad00cebb617facadd2b02b03e0edd3/6/ansible-6.build#L18

[felixfontein]

If there is a new major release before feature freeze (next week), it will also make it in. Only from feature freeze on the range is fully fixed.