New module: cnos_vlan and various utility files #21107

dkasberg · 2017-02-07T20:25:56Z

First module: cnos_vlan and various utility files. First 1 of 17 CNOS modules

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

cnos_vlan module

ANSIBLE VERSION

ansible 2.3.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides

SUMMARY

mattclay · 2017-02-08T03:50:12Z

lib/ansible/module_utils/cnos.py

+        command = command + bgpAFArg1 + " reflection " 
+    elif(bgpAFArg1 == "dampening"):
+        command = command + bgpAFArg1 + " "
+        if(bg


CI failure due to python 2.4 syntax error:

2017-02-08 01:52:26 Compiling ./lib/ansible/module_utils/cnos.py ... 2017-02-08 01:52:26 File "./lib/ansible/module_utils/cnos.py", line 2829 2017-02-08 01:52:26 with open(errorFile, 'r') as f: 2017-02-08 01:52:26 ^ 2017-02-08 01:52:26 SyntaxError: invalid syntax

We have fixed this issue that was reported by shippable in cnos.py and have updated the branch. We have not used shippable yet, but have just enabled our repo to have shippable check it, however, still trying to figure out a simple shippable.yml file to have it scan our Python files.

You can run the tests locally using docker, rather than using Shippable. Take a look at the integration test documentation.

dkasberg · 2017-02-08T16:19:18Z

I'm not able to determine if there is an issue that needs to be fixed based on the check that has failed. Can you help me understand?

dkasberg · 2017-02-08T16:29:23Z

I think I found the error now...
2017-02-08 08:29:36 File "./lib/ansible/module_utils/cnos.py", line 2713
2017-02-08 08:29:36 except Exception,e:
2017-02-08 08:29:36 ^
2017-02-08 08:29:36 SyntaxError: invalid syntax
2017-02-08 08:29:36
2017-02-08 08:29:36 ERROR: Command "python3.5 -m compileall -fq -x /.tox/ ./lib/ansible/module_utils/cnos.py ./lib/ansible/module_utils/cnos_devicerules.py ./lib/ansible/module_utils/cnos_errorcodes.py ./lib/ansible/modules/network/cnos/init.py ./lib/ansible/modules/network/cnos/cnos_vlan.py ./lib/ansible/utils/module_docs_fragments/cnos.py" returned exit status 1.

sivel · 2017-02-08T16:31:56Z

@dkasak See http://docs.ansible.com/ansible/dev_guide/developing_modules_python3.html#exceptions for using get_exception to properly handle exceptions for py2.4->py3

dkasak · 2017-02-08T19:16:03Z

@sivel, I think you wanted to mention @dkasberg.

ansibot · 2017-02-09T18:27:53Z

@dkasberg this PR contains the following merge comits:

6927290

Please rebase your branch to remove these commits.

click here for bot help

dkasberg · 2017-02-15T15:47:39Z

We have several modules that share the same set of options so we were advised to move all of them to ansible\lib\ansible\utils\module_docs_fragments\cnos.py. This module has no options specific to it. All the options are defined in the module_docs_fragments\cnos.py.

Please advise how to proceed. Should we move all the common options back into each of the individual module so this passes the 'ansible_doc' check in Shippable?

dkasberg · 2017-02-15T16:35:54Z

Additional comment to the above.

As I mentioned all options for this module have been moved to module_utils since they are in common with all our other modules, but we do have other variables that need to be input to the module. Refer to our documentation from our release last January in the link below As you can see we have an Options section there but also section called "Overloaded Variables". How do we accommodate this in Ansible's documentation guidelines? Should we convert them to the Options format? If that is the case, it may be difficult for the user to understand the various ways to input these variables....

http://systemx.lenovofiles.com/help/index.jsp?topic=%2Fcom.lenovo.switchmgt.ansible.doc%2Fcnos_vlan.html

FYI, we will be changing the documentation on our website once our modules are included in the Ansible product.

gundalow · 2017-02-15T16:47:16Z

Had to dig (I'll update the docs) options: {} is how you add in empty options.

gundalow · 2017-02-15T16:49:39Z

lib/ansible/modules/network/cnos/cnos_vlan.py

+     help/index.jsp?topic=%2Fcom.lenovo.switchmgt.ansible.doc%2Fansible_for_
+     cnos.html)
+version_added: "2.3"
+Options:


Please change to:
options: {}

gundalow · 2017-02-15T16:50:02Z

lib/ansible/modules/network/cnos/cnos_vlan.py

+     cnos.html)
+version_added: "2.3"
+Options:
+    - The following is a table depicting how the overloaded variables are used


From here to the end of the DOCUMENTAION block needs deleting

gundalow · 2017-02-15T17:00:43Z

lib/ansible/modules/network/cnos/cnos_vlan.py

@@ -48,6 +48,8 @@
     cnos.html)
 version_added: "2.3"
 Options:
+    {}
+
    - The following is a table depicting how the overloaded variables are used


These other lines will not end up in the documentation, so I believe they need removing.

Are you talking about all the lines starting with:
"- The following is a table....
to where the EXAMPLE section starts?

Correct. The script that generates the HTML will only look at that the sections defined in docs.ansible.com/ansible/dev_guide/developing_modules_documenting.html

gundalow · 2017-02-15T17:09:35Z

lib/ansible/modules/network/cnos/cnos_vlan.py

+     This module uses SSH to manage network device configuration.
+     The results of the operation can be viewed in results directory.
+     To know more about this module from Lenovo and customizing them for your
+     use cases, please visit our [User Guide](http://systemx.lenovofiles.com/


Please replace with

`U](http://systemx.lenovofiles.com/help/index.jsp?topic=%2Fcom.lenovo.switchmgt.ansible.doc%2Fansible_for_cnos.html)

dkasberg · 2017-02-15T20:55:45Z

I'm stuck trying to fix this error reported by this check: 'ansible-doc cnos_vlan'
ERROR! module cnos_vlan missing documentation (or could not parse documentation): 'options'
As I mentioned before, this module's options are in the common utils/module_doc_fragments/cnos.py file.

gundalow suggested using:
options: {}

But this fails as well.

sivel · 2017-02-15T21:17:33Z

It looks like you specified Options instead of options.

mattclay · 2017-02-15T23:12:28Z

CI failure due to module validation failure:

2017-02-15 22:45:28 ============================================================================
2017-02-15 22:45:28 lib/ansible/modules/network/cnos/cnos_vlan.py
2017-02-15 22:45:28 ============================================================================
2017-02-15 22:45:28 TRACE:
2017-02-15 22:45:28     while scanning a simple key
2017-02-15 22:45:28       in "<string>", line 6, column 3:
2017-02-15 22:45:28           [inventory_hostname]['username'] ... 
2017-02-15 22:45:28           ^
2017-02-15 22:45:28     could not find expected ':'
2017-02-15 22:45:28       in "cnos_vlan.EXAMPLES", line 58, column 35:
2017-02-15 22:45:28           [inventory_hostname]['username']}}  password={{ hostvars[invento ... 
2017-02-15 22:45:28                                           ^
2017-02-15 22:45:28 ERROR: EXAMPLES is not valid YAML. Line 58 column 35

gundalow · 2017-02-16T08:12:52Z

Merged :)

sivel · 2017-02-15T18:42:12Z

lib/ansible/module_utils/cnos.py

+    retVal = ""
+    command = "interface "
+    newPrompt = prompt
+    if(interfaceArg1 == "port-aggregation"):


There are many examples of this throughout this PR, however 2 things here:

The parentheses are not needed

There should be a space between if and (

My personal opinion, is that where unnecessary, such as in this example, the parentheses should be removed.

sivel · 2017-02-15T18:44:52Z

lib/ansible/module_utils/cnos.py

+def interfaceLevel2Config(
+    obj, deviceType, prompt, timeout, interfaceL2Arg1, interfaceL2Arg2,
+    interfaceL2Arg3, interfaceL2Arg4, interfaceL2Arg5, interfaceL2Arg6,
+        interfaceL2Arg7):


Ideally, all of the args should be indented to the same level as interfaceL2Arg7

Also, I'm not a fan of these variables. They seem pretty poorly named.

This applies throughout the codebase

sivel · 2017-02-15T18:47:36Z

lib/ansible/module_utils/cnos.py

+# EOM
+
+
+def interfaceLevel2Config(


This function, is really untestable. On a McCabe complexity scale, this is a 207, which means it contains at least 207 branches of code that would need to be independently tested. Generally speaking, in most scenarios, something below a 10-12 on a McCabe complexity is preferred.

My recommendation, is that this function needs to be split up, into many smaller functions.

sivel · 2017-02-15T18:50:09Z

lib/ansible/modules/network/cnos/cnos_vlan.py

+  outputfile=./results/test_vlan_{{ inventory_hostname }}_output.txt
+  vlanArg1='{{item.vlanArg1}}' vlanArg2='{{item.vlanArg2}}'
+  vlanArg3='{{item.vlanArg3}}'
+  with_items: "{{test_vlan_data1}}"


From what I can see here, these EXAMPLES are not valid YAML, and would not be operational if someone were to copy paste them.

They should be laid out in pure/full YAML such as:

- name: Test Vlan - Create a vlan, name it cnos_vlan: host: "{{ inventory_hostname }}" username: "{{ hostvars[inventory_hostname]['username'] }}"

mattclay reviewed Feb 8, 2017

View reviewed changes

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Feb 8, 2017

abadger changed the title ~~Cnos part1~~ New modules: cmos_vlan and cnos_command Feb 8, 2017

abadger removed the needs_triage Needs a first human triage before being processed. label Feb 8, 2017

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Feb 9, 2017

ansibot added the merge_commit This PR contains at least one merge commit. Please resolve! label Feb 9, 2017

more fixes, plus fix for python3

1ef6168

gundalow reviewed Feb 15, 2017

View reviewed changes

fix for empty options section, removed unnecessary documentation

4b32179

gundalow reviewed Feb 15, 2017

View reviewed changes

fix up documentation section

bf45f72

ansibot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Feb 15, 2017

Correct options capitalization

06da92b

ansibot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Feb 15, 2017

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Feb 15, 2017

fix example section, add options

5dae281

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Feb 15, 2017

Fix example section of the documentation

ae3629a

ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Feb 16, 2017

gundalow merged commit dbb4521 into ansible:devel Feb 16, 2017

dkasberg deleted the cnos-part1 branch February 16, 2017 16:38

dkasberg restored the cnos-part1 branch February 16, 2017 16:38

dkasberg deleted the cnos-part1 branch February 16, 2017 16:39

sivel reviewed Feb 16, 2017

View reviewed changes

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New module: cnos_vlan and various utility files #21107

New module: cnos_vlan and various utility files #21107

dkasberg commented Feb 7, 2017 •

edited

Loading

mattclay Feb 8, 2017

dkasberg Feb 8, 2017

mattclay Feb 8, 2017

dkasberg commented Feb 8, 2017

dkasberg commented Feb 8, 2017

sivel commented Feb 8, 2017

dkasak commented Feb 8, 2017

ansibot commented Feb 9, 2017

dkasberg commented Feb 15, 2017

dkasberg commented Feb 15, 2017

gundalow commented Feb 15, 2017

gundalow Feb 15, 2017

gundalow Feb 15, 2017 •

edited

Loading

gundalow Feb 15, 2017

dkasberg Feb 15, 2017

gundalow Feb 15, 2017

gundalow Feb 15, 2017

dkasberg commented Feb 15, 2017

sivel commented Feb 15, 2017

mattclay commented Feb 15, 2017

gundalow commented Feb 16, 2017

sivel Feb 15, 2017

sivel Feb 15, 2017

sivel Feb 15, 2017

sivel Feb 15, 2017

New module: cnos_vlan and various utility files #21107

New module: cnos_vlan and various utility files #21107

Conversation

dkasberg commented Feb 7, 2017 • edited Loading

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

SUMMARY

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkasberg commented Feb 8, 2017

dkasberg commented Feb 8, 2017

sivel commented Feb 8, 2017

dkasak commented Feb 8, 2017

ansibot commented Feb 9, 2017

dkasberg commented Feb 15, 2017

dkasberg commented Feb 15, 2017

gundalow commented Feb 15, 2017

Choose a reason for hiding this comment

gundalow Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkasberg commented Feb 15, 2017

sivel commented Feb 15, 2017

mattclay commented Feb 15, 2017

gundalow commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkasberg commented Feb 7, 2017 •

edited

Loading

gundalow Feb 15, 2017 •

edited

Loading