search

CiscoDevNet / ansible-nso

GNU General Public License v3.0
9 stars 10 forks source link

Inclusion in Ansible 2.10.x #4

Open felixfontein opened 3 years ago

felixfontein commented 3 years ago

I was looking at this collcetion a bit. I have some questions/points:

  1. example in https://github.com/CiscoDevNet/ansible-nso/blob/master/plugins/modules/nso_verify.py does not use FQCN
  2. what's https://github.com/CiscoDevNet/ansible-nso/tree/master/tests/sanity/unused_ignores ?
  3. do you have a CI running at least the sanity tests with ansible-base's stable-2.10 and devel branches?

Besides that, LGTM.

CC @gundalow @abadger

gundalow commented 3 years ago

If you create a PR with this lines https://github.com/ansible-collections/collection_template/blob/main/.github/workflows/ansible-test.yml#L1-L116 in a new file called .github/workflows/ansible-test.yml then you will get Sanity and Unit tests running.

Any issues please should out and we can help.

jabelk commented 3 years ago

The examples are aligned to the NSO DevNet reservable sandbox instance and don't require a full FQCN

felixfontein commented 3 years ago

@jabelk examples are targeted at end-users, who will not be able to use the modules from the collections when they don't explicitly use FQCNs. I would assume that people will also use this collection outside your reservable sandbox.

Also, this is a mandatory requirement for inclusion in Ansible, see All module and plugin EXAMPLES MUST: in: https://github.com/ansible-collections/overview/blob/main/collection_requirements.rst#documentation

jabelk commented 3 years ago

Ah, I misunderstood. You mean the module call needs the full collection.modulename, not that the network device needs some type of renaming.

jabelk commented 3 years ago

I added cisco.nso. namespace to the module example, thanks!

mamullen13316 commented 3 years ago

@felixfontein prior to seeing your last comment on PR #142 I had submitted a new PR https://github.com/ansible/ansible/pull/72753 which I thought was possibly necessary for the migration based on further analysis of the Onyx module migration.

felixfontein commented 3 years ago

@mamullen13316 that's the very last thing that can be merged, and only after (a) this collection is included in Ansible 2.10, and (b) ansible-collections/community.network#142 is merged. (And that PR is only relevant for ansible-core 2.12+.)

mamullen13316 commented 3 years ago

@jabelk going through the checklist it seems that we need to convert this repo to using branch name of main instead of master. Would there be impact to others working on this repo, DevNet labs, or anything else that depends on the branch named master being present?

felixfontein commented 3 years ago

@mamullen13316 I don't think that's a requirement for collection repos outside the github.com/ansible-collections namespace. It is nice if collections outside of that namespace also do it, but not required to be part of Ansible.

mamullen13316 commented 3 years ago

@felixfontein thanks for the clarification. In that case I believe we have satisfied the requirements in the checklist.

felixfontein commented 3 years ago

@mamullen13316 I think #5 needs to be addressed first.

felixfontein commented 3 years ago

Ok, I think everything's fine now. The only thing that is a bit strange is the https://github.com/CiscoDevNet/ansible-nso/tree/master/tests/sanity/unused_ignores directory, I think it can be safely deleted.

Maybe do another release (1.0.2) so that all fixes are included in the latest release? Then I think this collection can then be added to Ansible 2.10.

@gundalow what do you think?

gundalow commented 3 years ago

Yup, could you please delete that directory and release the next version?

Thanks for all the work that's been done here, and being responsive to the discussion.

mamullen13316 commented 3 years ago

Deleted the unused_ignores and updated to 1.0.2 release. You're welcome and thank you both for your help!

mamullen13316 commented 3 years ago

@felixfontein should I do a PR on ansible-build-data to have cisco.nso included in https://github.com/ansible-community/ansible-build-data/blob/main/2.10/ansible.in?

mamullen13316 commented 3 years ago

@felixfontein thanks for your help so far getting the nso modules migrated from community.network to their new home at cisco.nso. Could you provide any guidance as to what we need to do in order to point the documentation on docs.ansible.com to the new repo? It appears the docs are still pointing at community.network. For example: https://docs.ansible.com/ansible/latest/collections/community/network/nso_config_module.html#ansible-collections-community-network-nso-config-module

felixfontein commented 3 years ago

@mamullen13316 the docs are for the latest 2.10.x release, and for 2.10 the modules are staying in community.network. The redirect will only happen for community.network 2.0.0, which will be included in Ansible 3.0.0 (to be released end of January / beginning of February). Ansible 2.10.5 (to be released today) will include the cisco.nso module, so users can manually use the modules from that collection already - and once the docsite is updated (latest tomorrow), the docs for cisco.nso will be in https://docs.ansible.com/ansible/latest/collections/cisco/nso/