New collection review: cisco.dnac

[racampos]

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

• Ansible Galaxy: https://galaxy.ansible.com/cisco/dnac
• Github repo: https://github.com/cisco-en-programmability/dnacenter-ansible
• Github handles: @racampos @jbogarin

[racampos]

The collection satisfies all the requirements and is ready for review

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

Feel free to ask any questions and provide feedback. Thanks in advance for your understanding.

For details about the following points, refer to the Collection Requirements.

---

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
  Comment (MUST fix): I could not find any information about this in the repository. Some examples: https://sensu.github.io/sensu-go-ansible/release_policy.html, https://github.com/ansible-collections/community.sops#releasing-versioning-and-deprecation
• 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
  Comment: Galaxy is missing the 1.0.3 release that is tagged in the GitHub repo. I guess someone created a tag and never published the artifact? In any case, this is a minor issue and will not block the inclusion process as far as I am concerned.

Standards and documentation:

• adheres to semantic versioning
• follows licensing rules
• follows the Ansible documentation standards and the style guide
  Comment (MUST fix): All modules are missing return value documentation. And while this is OK if the modules only return back standard data, but the dnac modules seem to return back at least dnac_response custom field.
• follows development conventions
  Comment (MUST fix): All modules in the collection are a mix of a regular and an info module (state: query parameter combination). In an ideal world, all regular modules would enforce the desired state on the target host, and info modules would query the current state of things.
• 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)
  Comment (MUST fix): The collection only supports Python >= 3.5 (because the underlying SDK does not support Python 2), but it fails to document this in the requirements section of the documentation.
• Only uses recognized plugin types in the plugins/ directory.
• 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
  Comment (MUST fix): Collection is missing dependency on the ansible.utils collection that is being used in the code.
• 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
  Comment (MUST fix): Collection fails some import tests on what will become ansible-core-2.11. The code needs to be able to handle missing SDK and ansible.utils stuff gracefully.
• if test/sanity/ignore*.txt exists, it does not contain error codes listed here
  Comment: There are some ignores in the ingore-2.10.txt that should be fixed in the code and not ignored (https://github.com/cisco-en-programmability/dnacenter-ansible/blob/main/tests/sanity/ignore-2.10.txt#L213-L228).
• has CI tests up and running against each of the "major versions" of ansible-base/ansible-core that the collection supports
  Comment: Because ansible 4.0.0 will use ansible-core, this collection needs to start testing against the development branch as soon as possible (or against the stable-2.11 branch once that is created). There are some sanity tests that fail on that version.
• 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]

That's the reason we decided to replace the usual present/absent/query states with create/delete/update/query states and let the user determine if an object needs to be updated or created from scratch.

How do you expect users to determine if they need to do the create or update action? If the code cannot determine this, users will also have a hard time using the modules. Right now, modules are just a thin wrapper around the API, which is not the best option since YAML is probably the worst "programming language" available.

The https://docs.ansible.com/ansible/devel/dev_guide/developing_modules_best_practices.html#scoping-your-module-s section of the documentation describes how ansible modules should be scoped.

[cidrblock]

In an ideal world, I think all REST APIs would allow for idempotent operations, and round trip GET/PUT/POST operations with the same data structures....but

In this case it seems the DNAC API is a constraint limiting what can be feasibly supported with the modules.

In cases like this, create/delete/update/query make complete sense, have consistently been implemented throughout the collection, and do not introduce any ambiguity with regard to underlying API operation. I would consider these modules necessary building blocks that need to be in place for the community to start developing higher order roles that might parallel different DNAC workflows.

+1 on the approach and use of state here.

[gundalow]

@racampos Thank you for this collection inclusion request.

1. Have many of your users been using this collection so far?
2. What feedback have you received from your users have they managed to write Ansible roles that they can safely run multiple times? Has anything been confusing to them?
3. (building on the point above. How do you think the documentation can be improved to make the approach suggested clearer for your future users?

[tadeboro]

In an ideal world, I think all REST APIs would allow for idempotent operations, and round trip GET/PUT/POST operations with the same data structures....but

In this case it seems the DNAC API is a constraint limiting what can be feasibly supported with the modules.

In cases like this, create/delete/update/query make complete sense, have consistently been implemented throughout the collection, and do not introduce any ambiguity with regard to underlying API operation. I would consider these modules necessary building blocks that need to be in place for the community to start developing higher order roles that might parallel different DNAC workflows.

The state: query option is a no-go. The docs I linked clearly state: "Do not add get, list or info state options to an existing module - create a new _info or _facts module."

As for the state: create/delete/update, those indicate that we are just wrapping an underlying API and expect users to program in YAML. (And the readme makes this really clear: https://github.com/cisco-en-programmability/dnacenter-ansible#releasing-versioning-and-deprecation - modules will be updated in sync with Python SDK, so they offer almost no abstraction whatsoever.)

For example, if I want to make sure the application set is in the desired state, I would assume I need to do something like this:

- name: Fetch resource
  cisco_dnac.application_set:
    state: query
    name: foo
  register: app_set

- name: Upsert resource
  cisco_dnac.application_set:
    state: "{{ (app_set.dnac_response.foo is defined) | ansible.builtin.ternary('update', 'create') }}"
    name: foo
    more: params
    go: here

If this example can be made to work, then I see no reason why the module itself should not do that using a real programming language. If this cannot be made to work because the API does not return back enough information about the resource, then I am not sure who I am supposed to use the DNAC API at all.

Or am I missing something here?

[felixfontein]

As for the state: create/delete/update, those indicate that we are just wrapping an underlying API and expect users to program in YAML. (And the readme makes this really clear: https://github.com/cisco-en-programmability/dnacenter-ansible#releasing-versioning-and-deprecation - modules will be updated in sync with Python SDK, so they offer almost no abstraction whatsoever.)

This are exactly the kind of modules that Ansible never wanted while everything was in ansible/ansible. Quoting directly from https://docs.ansible.com/ansible/devel/dev_guide/developing_modules.html#should-you-develop-a-module:

Modules should typically encompass much of the logic for interacting with a resource. A lightweight wrapper around an API that does not contain much logic would likely cause users to offload too much logic into a playbook, and for this reason the module would be rejected. Instead try creating multiple modules for interacting with smaller individual pieces of the API.

@tadeboro:

If this example can be made to work, then I see no reason why the module itself should not do that using a real programming language.

I fully agree. As long as the behavior (either always update, or the behavior I sketch further below) is documented, this would be a lot more useful to end-users.

@tadeboro:

If this cannot be made to work because the API does not return back enough information about the resource, then I am not sure who I am supposed to use the DNAC API at all.

I've seen modules for APIs where either not enough information is available, cannot be retrieved reliably, or where obtaining that information would take too many resources (time, bandwidth, memory, ...). These usually document the options that are not used for idempotence checks, and offer a force: true option which allows to force updates. While such an interface isn't perfect either, it is a lot better than basically only offering distinct create/update operations.

[tadeboro]

Just a reminder: collections that want to be part of the Ansible 4.0.0 package need to get approval by 2021-04-26 (full schedule). This inclusion request still has some open issues that need to get addressed by then.

[tadeboro]

Last reminder for inclusion submission

Collections that want to be part of the Ansible 4.0.0 package need to get approval by 2021-04-26, which is just one week away (full schedule). Make sure you submit your collection for the final review a few days before the deadline (reviews take a non-trivial amount of time) and that your collection does meet all criteria listed in the requirements document.

[cidrblock]

RE, using an inventory to keep connection specific information and credentials for the host in play.

In this case, I prefer the use of the inventory, since these are attributes of the host in play (the DNAC server), the use of inventory seems very clean and appropriate.

(I see the DNAC python pkg handles it's own HTTP auth & connections so use of the HTTP API connection here doesn't make sense)

Yeah, technically, these were being used as module params and not parameters for a native connection plugin, but I think experience should have priority. I have DNAC servers in my inventory, and for each DNAC server I need to track 7 host specific attributes. host vars seems to be the right approach vs. an external vars file.

+1 for the use of inventory to maintain vars that are specific to a DNAC host (independent of whether these vars are used by a action/module or connection plugin)

[tadeboro]

RE, using an inventory to keep connection specific information and credentials for the host in play.

In this case, I prefer the use of the inventory, since these are attributes of the host in play (the DNAC server), the use of inventory seems very clean and appropriate.

I wholeheartedly agree, but the current implementation is not the right way to go since it has the potential to confuse users that just build their mental image of how Ansible works. See below for practical example.

(I see the DNAC python pkg handles it's own HTTP auth & connections so use of the HTTP API connection here doesn't make sense)

I am having a hard time following the reasoning here. DNAC python package does handle auth and connection stuff for us, but it still has to do it once per task invocation (each task invocation creates a new instance of the DNAC client). The httpapi plugin allows us to create a single DNAC client instance at the start of the play and then reuse it for all task invocations. The fact that we did not have to write our own HTTP API client makes no difference here.

Yeah, technically, these were being used as module params and not parameters for a native connection plugin, but I think experience should have priority. I have DNAC servers in my inventory, and for each DNAC server I need to track 7 host specific attributes. host vars seems to be the right approach vs. an external vars file.

I must disagree here. The user experience is significantly worse for people that know how to use Ansible because it deviates severely from the standard. When I read the instructions in the readme file for the first time, I was pretty sure someone made a mistake when writing them.

The things that threw me off were:

1. There was no address in the inventory that would allow Ansible to connect to the host. But maybe playbook will specify some custom connection plugin that will fetch connection data from dnac_-prefixed variables?
2. The playbook did not use any special connection type, which means that ssh is in use. How the heck does this work then?

For reference, these are the instructions I am talking about: https://github.com/cisco-en-programmability/dnacenter-ansible/tree/1.0.4#use

Because I am a developer, I was able to read the code and learn what action plugins actually do to make things work (and even I had some troubles understanding its logic the first time I read the code). But regular Ansible users would not be so lucky. And because no other collection behaves this way, they would need to rely on developers to explain to them that the modules behave differently from every other module in the ecosystem.

This is why I suggested the httpapi connection plugin as an alternative. The custom connection type clearly marks that things will behave differently than what would be considered a normal operation (establishing an ssh connection to the remote host and performing some action on it). And documentation for that connection plugin will list relevant parameters and what variables we can use to serve as a source of parameter values.

+1 for the use of inventory to maintain vars that are specific to a DNAC host (independent of whether these vars are used by a action/module or connection plugin)

I strongly disagree here. No action plugin or module uses variables directly, and there is a reason for it: they should only use what the user explicitly passed to them in parameters. And even plugins do not use variables directly. The configuration parser just happens to be able to use them as a source of parameter values (just like module argument parsers can use environment variables as a fallback option).

[cidrblock]

RE, using an inventory to keep connection specific information and credentials for the host in play.
In this case, I prefer the use of the inventory, since these are attributes of the host in play (the DNAC server), the use of inventory seems very clean and appropriate.

I wholeheartedly agree, but the current implementation is not the right way to go since it has the potential to confuse users that just build their mental image of how Ansible works. See below for practical example.

I do not believe that if the host in play is mydnacserver.company.net storing it's connection detail specifics in the inventory as attributes of the corresponding host entry leads to any confusion, this is the ansible pattern. I do agree there may be an opportunity to align the attribute names to be closer to the typical ansible attribute names may be worth a discussion. (ansible_host vs dnac_host)

(I see the DNAC python pkg handles it's own HTTP auth & connections so use of the HTTP API connection here doesn't make sense)

I am having a hard time following the reasoning here. DNAC python package does handle auth and connection stuff for us, but it still has to do it once per task invocation (each task invocation creates a new instance of the DNAC client). The httpapi plugin allows us to create a single DNAC client instance at the start of the play and then reuse it for all task invocations. The fact that we did not have to write our own HTTP API client makes no difference here.

I agree, and it is unfortunate that the use of client libraries does often cause the library and connection to be loaded and initiated with each task. I've seen some work arounds done here but nothing that I would consider a standard (AnsibleTurboModule in the vcenter rest collection)

I've only looked into it briefly but the DNAC python library makes it's own HTTP connections https://github.com/CiscoDevNet/DNAC-Python-SDK/blob/878369d3642f4159748ab67f844d6da83acf744f/python_client/rest.py#L207 where as the HTTP API connection plugin does the same: https://github.com/ansible-collections/ansible.netcommon/blob/main/plugins/connection/httpapi.py#L330. The 2 seem to overlap with capabilities and intended use and from a quick look it would be difficult to leverage them together.

Yeah, technically, these were being used as module params and not parameters for a native connection plugin, but I think experience should have priority. I have DNAC servers in my inventory, and for each DNAC server I need to track 7 host specific attributes. host vars seems to be the right approach vs. an external vars file.

I must disagree here. The user experience is significantly worse for people that know how to use Ansible because it deviates severely from the standard. When I read the instructions in the readme file for the first time, I was pretty sure someone made a mistake when writing them.

Looking back at the original README in the repo, I see a yaml based inventory file, with attributes for each host. These attributes describe the host, port, username and password, again I personally do not find this confusing.

The things that threw me off were:

1. There was no address in the inventory that would allow Ansible to connect to the host. But maybe playbook will specify some custom connection plugin that will fetch connection data from dnac_-prefixed variables?

Based on a quick look at the DNAC python SDK, it appears to have an "embedded" connection manager, that is connections are handled within the SDK and therefore no ansible connection plugin necessary.

2. The playbook did not use any special connection type, which means that ssh is in use. How the heck does this work then?

Again, it appears the DNAC python client handles and manages its own connection so no ansible connection is necessary to use the DNAC modules.

For reference, these are the instructions I am talking about: https://github.com/cisco-en-programmability/dnacenter-ansible/tree/1.0.4#use

Because I am a developer, I was able to read the code and learn what action plugins actually do to make things work (and even I had some troubles understanding its logic the first time I read the code). But regular Ansible users would not be so lucky. And because no other collection behaves this way, they would need to rely on developers to explain to them that the modules behave differently from every other module in the ecosystem.

I think action plugins makes sense in this scenario because the HTTP connections are made from the control node and no module necessary. The use of a module would require the target host to be localhost, which is not really the host being managed.

This is why I suggested the httpapi connection plugin as an alternative. The custom connection type clearly marks that things will behave differently than what would be considered a normal operation (establishing an ssh connection to the remote host and performing some action on it). And documentation for that connection plugin will list relevant parameters and what variables we can use to serve as a source of parameter values.

I understand the HTTP API connection to be useful when URLs are crafted and can be sent to the host. With the use of a python SDK I think it can't provide much value here and would probably require significant changes to the python SDK to be integrated.

I agree ansible has an opportunity here to better explain and depict a model in which swagger codegen generated python SDKs can be used along side ansible, without repeated connection teardown and reestablishment and reloading, unfortunately I don't think this would be a small undertaking, nor should the burden of that be on this collection...

+1 for the use of inventory to maintain vars that are specific to a DNAC host (independent of whether these vars are used by a action/module or connection plugin)

I strongly disagree here. No action plugin or module uses variables directly, and there is a reason for it: they should only use what the user explicitly passed to them in parameters. And even plugins do not use variables directly. The configuration parser just happens to be able to use them as a source of parameter values (just like module argument parsers can use environment variables as a fallback option).

I have not audited the use of variable use across all action plugins, so I cannot agree or disagree here. With the connection embedded in the python client and no ansible connection plugin necessary, I think having to provide connection details at the task or task default level is unnecessary. Again, IMO, keeping connection details in the inventory as attributes of the host in play makes sense to me, and if the lack of an ansible connection due to the nature of the python client eliminates the need for a connection plugin and then results in the use of host vars by the action plugin, I think it's reasonable.

[tadeboro]

I do not believe that if the host in play is mydnacserver.company.net storing it's connection detail specifics in the inventory as attributes of the corresponding host entry leads to any confusion, this is the ansible pattern. I do agree there may be an opportunity to align the attribute names to be closer to the typical ansible attribute names may be worth a discussion. (ansible_host vs dnac_host)

The problem I see here is that those "parameters" are not documented anywhere (because they are not parameters). And because no custom connection plugin is in use, users will get confused when they start looking for the ssh connection plugin option that would use dnac_host variable as a source. As stated before, this is not confusing only if you know almost nothing about Ansible. For users that do know how Ansible operates in general, this becomes confusing real fast.

I agree, and it is unfortunate that the use of client libraries does often cause the library and connection to be loaded and initiated with each task. I've seen some work arounds done here but nothing that I would consider a standard (AnsibleTurboModule in the vcenter rest collection)

I've only looked into it briefly but the DNAC python library makes it's own HTTP connections https://github.com/CiscoDevNet/DNAC-Python-SDK/blob/878369d3642f4159748ab67f844d6da83acf744f/python_client/rest.py#L207 where as the HTTP API connection plugin does the same: https://github.com/ansible-collections/ansible.netcommon/blob/main/plugins/connection/httpapi.py#L330. The 2 seem to overlap with capabilities and intended use and from a quick look it would be difficult to leverage them together.

This was my bad. I looked at the httpapi plugins more than a year ago and I forgot that they delegate all the interaction with the remote API endpoint to the httpapi connection plugin. So in this case, something like https://steampunk.si/blog/let-us-give-ansible-a-rest/#connection-plugin would be needed actually if the persistent connection is something we would be interested in.

Looking back at the original README in the repo, I see a yaml based inventory file, with attributes for each host. These attributes describe the host, port, username and password, again I personally do not find this confusing.

But when ssh connection plugin is in use, users expect that modules will run on that host. Or do we not expect users to know anything about how Ansible executes things and instruct them to blindly rely on instructions someone provided them?

Based on a quick look at the DNAC python SDK, it appears to have an "embedded" connection manager, that is connections are handled within the SDK and therefore no ansible connection plugin necessary.

1. The playbook did not use any special connection type, which means that ssh is in use. How the heck does this work then?

Again, it appears the DNAC python client handles and manages its own connection so no ansible connection is necessary to use the DNAC modules.

Up until now, I considered the connection plugin to be an essential part of each task execution because it allows me to determine where the module will run. For ssh connections, Ansible will run the module on the remote host, the local connection will run module on the control node, the httpapi connection will run the module on localhost, network_cli will ... Current implementation of DNAC modules breaks that assumption because it "uses" the ssh connection plugin and executes module on localhost.

The documentation does not mention that the DNAC collection does things in its own way. The fact that modules can only be run on localhost is not mentioned anywhere. And as said before, there is no way to determine what variables are used to establish the connection because they are neither module parameters nor plugin options.

I think action plugins makes sense in this scenario because the HTTP connections are made from the control node and no module necessary. The use of a module would require the target host to be localhost, which is not really the host being managed.

What host means is determined by what connection plugin is in use. For ssh connection, this means the location of the module execution. For httpapi connection, this means API target that the module will contact from localhost.

I have not audited the use of variable use across all action plugins, so I cannot agree or disagree here. With the connection embedded in the python client and no ansible connection plugin necessary, I think having to provide connection details at the task or task default level is unnecessary. Again, IMO, keeping connection details in the inventory as attributes of the host in play makes sense to me, and if the lack of an ansible connection due to the nature of the python client eliminates the need for a connection plugin and then results in the use of host vars by the action plugin, I think it's reasonable.

How do we document intended usage in this case? I have a hard time finding the place to document the inventory variables. Usually, those would be documented in either connection plugin docs or as part of the module parameter. And neither applies here.

[cidrblock]

The problem I see here is that those "parameters" are not documented anywhere (because they are not parameters). And because no custom connection plugin is in use, users will get confused when they start looking for the ssh connection plugin option that would use dnac_host variable as a source. As stated before, this is not confusing only if you know almost nothing about Ansible. For users that do know how Ansible operates in general, this becomes confusing real fast.

You raise an excellent point here, the documentation. Although some of this could be documented in the collection README, once added to the community package of ansible, the collection README is not readily available. I think this there is an opportunity to enhance each plugins doc's more below.

But when ssh connection plugin is in use, users expect that modules will run on that host. Or do we not expect users to know anything about how Ansible executes things and instruct them to blindly rely on instructions someone provided them?

Up until now, I considered the connection plugin to be an essential part of each task execution because it allows me to determine where the module will run. For ssh connections, Ansible will run the module on the remote host, the local connection will run module on the control node, the httpapi connection will run the module on localhost, network_cli will ... Current implementation of DNAC modules breaks that assumption because it "uses" the ssh connection plugin and executes module on localhost.

Having done only quick review of the code, I could not find any use of the SSH connection plugin.

How do we document intended usage in this case? I have a hard time finding the place to document the inventory variables. Usually, those would be documented in either connection plugin docs or as part of the module parameter. And neither applies here.

I really like that you brought attention to the documentation part here. I think there is a opportunity to improve the UX WRT connection details.

Some ideas:

1. In each the module documentation for each action plugins, add a Note: that explains the plugin runs on the control node and does not use any ansible connection plugins but instead the embedded connection manager from the DNAC python SDK
2. Also add a note explaining the 4 or so host variable that are used for the connection, that can be in the inventory or provided directly as task vars.
3. In the examples, show the use of task vars in at least one task, and a sample inventory stanza #'ed out for another.

I was originally wondering if the typical ansible attr should be used for the DNAC connection, but after reading through your comments, I think that would not be a good idea. I'm thinking here of a scenario where the user, in the same playbook, may want to indeed use the SSH plugin for OS level tasks, and later use the DNAC plugins for DNAC related work. It's probably safe to assume the credentials won't be the same, and having them prefaced with dnac_ makes more sense I think.

Aside from a real opportunity to improve the documentation as you suggested, I know Cisco is going to stand behind this collection, and there are many users eager to see it in the community package and as part of the platform available on AH.

Thanks for holding a consistently high bar for the quality of the ansible ecosystem content!

[tadeboro]

The cisco.dnac Ansible Collection was not accepted into the Ansible community package. The relevant part of the community meeting starts here.

The next opportunity for inclusion will be in about six months for Ansible 5.0.0.

[gundalow]

@racampos @wastorga Hi, just checking in as I've not seen much activity here in getting cisco.dnac included in the Ansible package (sorry if I've missed it)

We are approaching the deadline for Ansible 5.0.0

2021-10-12
Last day for new collections to be submitted for inclusion in Ansible-5. Collections MUST be reviewed and approved before being included. There is no guarantee that we will review every collection. The earlier your collection is submitted, the more likely it will be that your collection will be reviewed and the necessary feedback can be addressed in time for inclusion.

[racampos]

Hello @gundalow. I'm happy to announce that we have modified the approach we originally took regarding the state parameter. We have split the modules up into modules that accept present/absent states and "info" modules, which are used just to retrieve information, therefore avoiding the controversial query state.

We also decided against using the inventory to pass parameters and instead we are specifying the parameters independently for each task.

We hope that with these changes our collection will be in line with Ansible's best practices and that it will be approved for inclusion in Ansible 5. Please let us know if there's any other modification that we should consider.

[racampos]

Hello @gundalow. Just to make sure you're aware of the changes we made to the collection and that they will be revised before the deadline.

[racampos]

Hello @gundalow, @tadeboro. Were you able to review the changes we made to this collection? Thanks!

[felixfontein]

Please note that the filename changelogs/changelogs.yaml will not be picked up by Ansible; you have to use changelogs/changelog.yaml (with one less s).

[felixfontein]

Also you aren't testing against stable-2.12, which would be important since Ansible 5 is based on ansible-core 2.12.

[racampos]

Hello @felixfontein. Thanks for your input. Both issues have been addressed. Now we're testing against stable-2.12 and we changed the changelog filename.

[racampos]

Hello @felixfontein, @gundalow, @tadeboro. We're concerned about the review process for this collection, as it has been idle for nearly two weeks. All issues were fixed long ago and it still hasn't been approved.

[wastorga]

Hello @felixfontein, @gundalow, @tadeboro.
I would appreciate it if you could respond.

Do we need to create a new discussion? We're concerned about the review process for this collection, as it has been idle. All issues were fixed long ago and it hasn't been reviewed again.

[gundalow]

Hi @wastorga
@Andersson007 from my team has kindly offered to review the remaining items.
Once approved, cisco.dnac can be added into the next release of the ansible package, no need to wait for the next Major ansible release.

[Andersson007]

yep, i'll try to take a look this week

[wastorga]

Thank you, @gundalow. Those are excellent news.
Thank you, @Andersson007.

[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
• 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
• 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)
  COMMENT: In README and in every module is said that it supports 3.5+, i've asked folks if that's enough.
• 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
• 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 created by development tools. Temporary files MUST be added to .gitignore.

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]

I've checked it from scratch - i confirm that the collection now satisfies the requirements (including those items spotted by Tadej)
+1 for its inclusion

[Andersson007]

Created an inclusion topic ansible-community/community-topics#101. If no major concerns are raised, it'll get included in Ansible 5 on 19 May 2022 or a bit later.

[felixfontein]

While looking at the changelog I noticed some entries in the past that are problematic. Please try to avoid such things in the future.

1. https://github.com/cisco-en-programmability/dnacenter-ansible/blob/main/changelogs/changelog.yaml#L119 is a new feature appearing in a patch release. According to semantic versioning, new features must appear in minor (or major) releases.
2. Same thing here: https://github.com/cisco-en-programmability/dnacenter-ansible/blob/main/changelogs/changelog.yaml#L418
3. Adding new features is not a major change. This basically would fill our porting guide (which is for porting playbooks to a new major Ansible release) with irrelevant entries: https://github.com/cisco-en-programmability/dnacenter-ansible/blob/main/changelogs/changelog.yaml#L276

[wastorga]

@felixfontein Understood, thank you for the feedback.

[Andersson007]

Included ansible-community/ansible-build-data#125, congratulations and thanks!