Fix for nxos_snmp_host issues

[saichint]

SUMMARY

fixes #39641

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

nxos_snmp_host

ANSIBLE VERSION

ansible 2.6.0 (devel fed20b825f) last updated 2018/02/15 12:51:12 (GMT -400)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /root/agents-ci/ansible/lib/ansible
  executable location = /root/agents-ci/ansible/bin/ansible
  python version = 2.7.6 (default, Oct 26 2016, 20:30:19) [GCC 4.8.4]

ADDITIONAL INFORMATION

• This PR fixes nxos_snmp_host issues #39641
• Integration tests are enhanced for various platforms and version 1,2 and 3.
• All tests pass on all platforms

[ansibot]

cc @GGabriele @jedelman8 @mattclay @mikewiebe @privateip @rahushen @rcarrillocruz @trishnaguha
click here for bot help

[mattclay]

New ignores should not be added. The underlying issue should be resolved. If the issue is in module_utils or docs fragments, the fix should be done in a separate PR.

[saichint]

This was done deliberately to maintain backward compatibility due to the previous code bugs.

Here is the long explanation:

The defaults for snmp_type and snmp_version are 'trap' and 'v1'.
There are 4 different CLIs for nxos_snmp_host.

snmp-server host 1.1.1.1 informs version 2c public udp-port 222
snmp-server host 1.1.1.1 use-vrf default udp-port 222
snmp-server host 1.1.1.1 filter-vrf management udp-port 222
snmp-server host 1.1.1.1 source-interface Ethernet1/1

So basically the user can add use-vrf, filter-vrf and source interface in different tasks without specifying the version and type if the host ip(and udp port) already exists (the first CLI). If we define the defaults in the argument_spec (which is what is needed if we have to get rid of the ignores), the defaults of v1 and trap type are used which is incorrect, as the type is 'inform' and version is '2c' for the existing configuration. So the configuration fails.

The only other way is to make the version and trap type as 'required' parameters but then older playbooks from previous versions cannot work any longer because they were not 'required' before.
Hence this ignore is added. If the code was written properly in the beginning this could have been avoided but I don't see any other way. And please remember that this PR resolves some very MAJOR issues.
We can make a decision to ignore the backward compatibility all together since this module is broken completely before. In that case, I can take make the version as type as 'required' and remove the new ignores.
Which option do you suggest?

[mattclay]

Removing the default from the argument spec is itself a non-backwards compatible change. Any playbook which previously relied on the default of 'trap' will now use None instead. However, if that is indeed the only way to fix the issue then that change is unavoidable.

However, in that case, the fix for the validation error is simple. Remove the default from the documentation so it matches the actual default.

The same is true for the version parameter as well. The default in the docs must match the actual default in the arg spec.

[saichint]

Not really. If you look at the code, the way it works is that, if there is no existing snmp host, internally the code will make the version as 1 and type as trap (even though they are not defined in the arg spec). If there is an existing host, it will use the values from that host. So 'None' will not be an issue.
However, if we remove the defaults from documentation to match the arg spec, then the defaults are 'truly' removed and user MUST specify them always which breaks the backward compatibility. Since you are saying that the doc MUST match the arg spec no matter what, I can do by removing the defaults all together for type and version. But as I mentioned, old playbooks won't work if they have no do not specify the version and type.
Just wanted to clarify this before making changes.

[mattclay]

Simply remove the default: entries from the version and snmp_type docs. The logic in the code requires no additional changes.

With that change the docs will match the defaults in the arg spec. Any other default-like behavior that occurs outside of the argument spec isn't a true default and should be documented in the description instead.

[saichint]

@mattclay Thanks for the comments. Made the changes and removed the E324 ignore.

[trishnaguha]

cherry-picked to 2.5