Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ericsson contribute feature and module #57353

Closed
wants to merge 21 commits into from

Conversation

Projects
None yet
6 participants
@ghost
Copy link

commented Jun 4, 2019

SUMMARY
ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME
   new file:   docs/docsite/rst/network/user_guide/platform_eric_eccli.rst
   modified:   lib/ansible/config/base.yml
   new file:   lib/ansible/module_utils/network/eric_eccli/__init__.py
   new file:   lib/ansible/module_utils/network/eric_eccli/eric_eccli.py
   new file:   lib/ansible/modules/network/eric_eccli/__init__.py
   new file:   lib/ansible/modules/network/eric_eccli/eric_eccli_command.py
   new file:   lib/ansible/plugins/action/eric_eccli.py
   new file:   lib/ansible/plugins/cliconf/eric_eccli.py
   new file:   lib/ansible/plugins/doc_fragments/eric_eccli.py
   new file:   lib/ansible/plugins/terminal/eric_eccli.py
   new file:   test/units/modules/network/eric_eccli/__init__.py
   new file:   test/units/modules/network/eric_eccli/eccli_module.py
   new file:   test/units/modules/network/eric_eccli/fixtures/configure_terminal
   new file:   test/units/modules/network/eric_eccli/fixtures/show_version
   new file:   test/units/modules/network/eric_eccli/test_eccli_command.py
ADDITIONAL INFORMATION
 Contribute  new feature and module to ansible, in order to support   
 Ericsson device command automated configuration. Have done a qualified test ansible required  on
 new module, example: use "ansible-test units --tox" and "pytest" tool to test.

Cheng You
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

@Ansible912, just so you are aware we have a dedicated Working Group for network.
You can find other people interested in this in #ansible-network on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ghost

This comment has been minimized.

Copy link
Author

commented Jun 4, 2019

apply approval to contribute these code to ansible, what need to do for me? I have done a test for module. It is ok!

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

test/units/modules/network/eric_eccli/test_eccli_command.py:1:20: trailing-whitespace Trailing whitespace

The test ansible-test sanity --test pylint [explain] failed with 13 errors:

lib/ansible/plugins/cliconf/eric_eccli.py:31:0: misplaced-future __future__ import is not the first non docstring statement
lib/ansible/plugins/cliconf/eric_eccli.py:83:35: ansible-bad-import-from Import Mapping from ansible.module_utils.common._collections_compat instead of collections
lib/ansible/plugins/cliconf/eric_eccli.py:100:0: trailing-newlines Trailing newlines
lib/ansible/plugins/terminal/eric_eccli.py:39:20: bad-whitespace Exactly one space required around assignment                 line=line.strip('\n')                     ^
lib/ansible/plugins/terminal/eric_eccli.py:41:33: bad-whitespace No space allowed before :                 if li == '[ERE]' :                                  ^
lib/ansible/plugins/terminal/eric_eccli.py:43:35: bad-whitespace No space allowed before :                 elif li == '[PRE]' :                                    ^
lib/ansible/plugins/terminal/eric_eccli.py:46:31: singleton-comparison Comparison to False should be 'not expr'
lib/ansible/plugins/terminal/eric_eccli.py:48:58: bad-whitespace Exactly one space required after comma                             new_re = re.compile(bytes(line,'ascii'))                                                           ^
lib/ansible/plugins/terminal/eric_eccli.py:52:32: singleton-comparison Comparison to False should be 'not expr'
lib/ansible/plugins/terminal/eric_eccli.py:53:36: singleton-comparison Comparison to False should be 'not expr'
lib/ansible/plugins/terminal/eric_eccli.py:56:32: singleton-comparison Comparison to False should be 'not expr'
lib/ansible/plugins/terminal/eric_eccli.py:57:36: singleton-comparison Comparison to False should be 'not expr'
lib/ansible/plugins/terminal/eric_eccli.py:103:0: trailing-newlines Trailing newlines

The test ansible-test sanity --test docs-build [explain] failed with 5 errors:

docs/docsite/rst/index.rst:0:0: unknown: 
docs/docsite/rst/index.rst:0:0: unknown: ----------------------------------
docs/docsite/rst/index.rst:0:0: unknown: Example CLI ``group_vars/eric_eccli.yml``
docs/docsite/rst/network/user_guide/platform_eric_eccli.rst:0:0: not-in-toc-tree: document isn't included in any toctree
docs/docsite/rst/network/user_guide/platform_eric_eccli.rst:41:0: warning: Title underline too short.

The test ansible-test sanity --test action-plugin-docs [explain] failed with 1 error:

lib/ansible/plugins/action/eric_eccli.py:0:0: action plugin has no matching module to provide documentation

The test ansible-test sanity --test boilerplate [explain] failed with 2 errors:

lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: missing: __metaclass__ = type
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test no-main-display [explain] failed with 1 error:

lib/ansible/plugins/terminal/eric_eccli.py:18:5: Display is a singleton, just import and instantiate

The test ansible-test sanity --test pep8 [explain] failed with 26 errors:

lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:114:22: W291 trailing whitespace
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:124:11: W291 trailing whitespace
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:125:39: W291 trailing whitespace
lib/ansible/plugins/cliconf/eric_eccli.py:100:1: W391 blank line at end of file
lib/ansible/plugins/doc_fragments/eric_eccli.py:6:1: E302 expected 2 blank lines, found 1
lib/ansible/plugins/terminal/eric_eccli.py:23:1: E302 expected 2 blank lines, found 1
lib/ansible/plugins/terminal/eric_eccli.py:39:21: E225 missing whitespace around operator
lib/ansible/plugins/terminal/eric_eccli.py:41:33: E203 whitespace before ':'
lib/ansible/plugins/terminal/eric_eccli.py:43:35: E203 whitespace before ':'
lib/ansible/plugins/terminal/eric_eccli.py:46:51: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
lib/ansible/plugins/terminal/eric_eccli.py:48:59: E231 missing whitespace after ','
lib/ansible/plugins/terminal/eric_eccli.py:52:54: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
lib/ansible/plugins/terminal/eric_eccli.py:53:58: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
lib/ansible/plugins/terminal/eric_eccli.py:56:54: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
lib/ansible/plugins/terminal/eric_eccli.py:57:58: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
lib/ansible/plugins/terminal/eric_eccli.py:69:1: E302 expected 2 blank lines, found 1
lib/ansible/plugins/terminal/eric_eccli.py:79:9: E265 block comment should start with '# '
lib/ansible/plugins/terminal/eric_eccli.py:81:9: E265 block comment should start with '# '
lib/ansible/plugins/terminal/eric_eccli.py:82:9: E265 block comment should start with '# '
lib/ansible/plugins/terminal/eric_eccli.py:83:9: E265 block comment should start with '# '
lib/ansible/plugins/terminal/eric_eccli.py:84:9: E265 block comment should start with '# '
lib/ansible/plugins/terminal/eric_eccli.py:85:9: E265 block comment should start with '# '
lib/ansible/plugins/terminal/eric_eccli.py:87:9: E265 block comment should start with '# '
lib/ansible/plugins/terminal/eric_eccli.py:89:9: E265 block comment should start with '# '
lib/ansible/plugins/terminal/eric_eccli.py:103:1: W391 blank line at end of file
test/units/modules/network/eric_eccli/test_eccli_command.py:1:21: W291 trailing whitespace

The test ansible-test sanity --test rstcheck [explain] failed with 1 error:

docs/docsite/rst/network/user_guide/platform_eric_eccli.rst:41:0: Title underline too short.

The test ansible-test sanity --test shebang [explain] failed with 5 errors:

lib/ansible/module_utils/network/eric_eccli/eric_eccli.py:0:0: file without shebang should not be executable
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: module should not be executable
lib/ansible/plugins/action/eric_eccli.py:0:0: file without shebang should not be executable
lib/ansible/plugins/doc_fragments/eric_eccli.py:0:0: file without shebang should not be executable
lib/ansible/plugins/terminal/eric_eccli.py:0:0: file without shebang should not be executable

The test ansible-test sanity --test validate-modules [explain] failed with 4 errors:

lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: E305 DOCUMENTATION.author: Invalid author for dictionary value @ data['author']. Got 'Ericsson IPOS OAM team'
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: E305 DOCUMENTATION.version_added: required key not provided @ data['version_added']. Got None
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: E307 version_added should be '2.9'. Currently None
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:117:9: E311 EXAMPLES is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:117:9: error EXAMPLES: syntax error: expected <block end>, but found '['

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Jun 4, 2019

Cheng You
@ghost

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

I have done some correction on module code.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

The test ansible-test sanity --test pylint [explain] failed with 5 errors:

lib/ansible/plugins/terminal/eric_eccli.py:47:31: singleton-comparison Comparison to False should be 'not expr'
lib/ansible/plugins/terminal/eric_eccli.py:53:32: singleton-comparison Comparison to False should be 'not expr'
lib/ansible/plugins/terminal/eric_eccli.py:54:36: singleton-comparison Comparison to False should be 'not expr'
lib/ansible/plugins/terminal/eric_eccli.py:57:32: singleton-comparison Comparison to False should be 'not expr'
lib/ansible/plugins/terminal/eric_eccli.py:58:36: singleton-comparison Comparison to False should be 'not expr'

The test ansible-test sanity --test action-plugin-docs [explain] failed with 1 error:

lib/ansible/plugins/action/eric_eccli.py:0:0: action plugin has no matching module to provide documentation

The test ansible-test sanity --test boilerplate [explain] failed with 2 errors:

lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: missing: __metaclass__ = type
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test no-main-display [explain] failed with 1 error:

lib/ansible/plugins/terminal/eric_eccli.py:18:5: Display is a singleton, just import and instantiate

The test ansible-test sanity --test pep8 [explain] failed with 6 errors:

lib/ansible/plugins/cliconf/eric_eccli.py:33:1: E265 block comment should start with '# '
lib/ansible/plugins/terminal/eric_eccli.py:47:51: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
lib/ansible/plugins/terminal/eric_eccli.py:53:54: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
lib/ansible/plugins/terminal/eric_eccli.py:54:58: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
lib/ansible/plugins/terminal/eric_eccli.py:57:54: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
lib/ansible/plugins/terminal/eric_eccli.py:58:58: E712 comparison to False should be 'if cond is False:' or 'if not cond:'

The test ansible-test sanity --test shebang [explain] failed with 6 errors:

lib/ansible/module_utils/network/eric_eccli/eric_eccli.py:0:0: should not be executable
lib/ansible/module_utils/network/eric_eccli/eric_eccli.py:0:0: should not have a shebang
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: module should not be executable
lib/ansible/plugins/action/eric_eccli.py:0:0: file without shebang should not be executable
lib/ansible/plugins/doc_fragments/eric_eccli.py:0:0: file without shebang should not be executable
lib/ansible/plugins/terminal/eric_eccli.py:0:0: file without shebang should not be executable

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: E305 DOCUMENTATION.author: Invalid author for dictionary value @ data['author']. Got 'Ericsson IPOS OAM team'
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:125:9: E311 EXAMPLES is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:125:9: error EXAMPLES: syntax error: could not find expected ':'

click here for bot help

Cheng You
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/plugins/terminal/eric_eccli.py:32:0: trailing-whitespace Trailing whitespace

The test ansible-test sanity --test action-plugin-docs [explain] failed with 1 error:

lib/ansible/plugins/action/eric_eccli.py:0:0: action plugin has no matching module to provide documentation

The test ansible-test sanity --test boilerplate [explain] failed with 2 errors:

lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: missing: __metaclass__ = type
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/plugins/terminal/eric_eccli.py:32:1: W293 blank line contains whitespace

The test ansible-test sanity --test shebang [explain] failed with 6 errors:

lib/ansible/module_utils/network/eric_eccli/eric_eccli.py:0:0: should not be executable
lib/ansible/module_utils/network/eric_eccli/eric_eccli.py:0:0: should not have a shebang
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: module should not be executable
lib/ansible/plugins/action/eric_eccli.py:0:0: file without shebang should not be executable
lib/ansible/plugins/doc_fragments/eric_eccli.py:0:0: file without shebang should not be executable
lib/ansible/plugins/terminal/eric_eccli.py:0:0: file without shebang should not be executable

click here for bot help

Cheng You
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

The test ansible-test sanity --test action-plugin-docs [explain] failed with 1 error:

lib/ansible/plugins/action/eric_eccli.py:0:0: action plugin has no matching module to provide documentation

The test ansible-test sanity --test boilerplate [explain] failed with 2 errors:

lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: missing: __metaclass__ = type
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

click here for bot help

Cheng You added some commits Jun 6, 2019

Cheng You
Cheng You
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

The test ansible-test sanity --test action-plugin-docs [explain] failed with 1 error:

lib/ansible/plugins/action/eric_eccli.py:0:0: action plugin has no matching module to provide documentation

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:150:1: E303 too many blank lines (4)

click here for bot help

Cheng You added some commits Jun 11, 2019

Cheng You
Cheng You
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/plugins/connection/network_cli.py:446:146: trailing-whitespace Trailing whitespace

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/plugins/connection/network_cli.py:446:147: W291 trailing whitespace

click here for bot help

Cheng You

@ansibot ansibot added core_review and removed needs_revision labels Jun 11, 2019

@ansible-zuul

This comment has been minimized.

Copy link

commented Jun 11, 2019

Build succeeded (third-party-check pipeline).

@Qalthos
Copy link
Contributor

left a comment

It looks like you copied one of the existing platforms to make this, but this means you have a lot of baggage that you don't actually need here.


_DEVICE_CONFIGS = {}

eric_eccli_provider_spec = {

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 11, 2019

Contributor

It doesn't look like you're even using connection: local, why is any of this here?

This comment has been minimized.

Copy link
@ghost

ghost Jun 14, 2019

Author

Thank your advise, I will delete it.

new_pres = []
mode = 0
file = None
if platform.python_version().startswith('3'):

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 11, 2019

Contributor

PY2 and PY3 are booleans importable from ansible.module_utils.six

This comment has been minimized.

Copy link
@ghost

ghost Jun 14, 2019

Author

Thank you. I will adopt this import function instead of above.

return module._eric_eccli_capabilities


def check_args(module, warnings):

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 11, 2019

Contributor

You don't need this function

module.fail_json(msg=to_text(exc))


def normalize_interface(name):

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 11, 2019

Contributor

Does this function do anything at all for your platform, or was this just copied from another module_utils file?

| **Returned Data Format** | ``stdout[0].`` |
+---------------------------+-----------------------------------------------+

For legacy playbooks, ERIC_ECCLI still supports ``ansible_connection: local``. We recommend modernizing to use ``ansible_connection: network_cli`` as soon as possible.

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 11, 2019

Contributor

Your module does not support connection: local

This comment has been minimized.

Copy link
@danielmellado

danielmellado Jun 12, 2019

Contributor

if the module does not support it, please remove the documentation regarding to it



def parse_commands(module, warnings):
command = ComplexList(dict(

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 11, 2019

Contributor

You should probably use transform_commands from ansible.module_utils.network.common.utils instead.

return cfg


def to_commands(module, commands):

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 11, 2019

Contributor

This function isn't used, and when you implement the same functionality in eric_eccli_command you probably shouldn't be doing it yourself anyway.

def get(self, command=None, prompt=None, answer=None, sendonly=False, output=None, check_all=False):
if not command:
raise ValueError('must provide value of command to execute')
if output:

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 11, 2019

Contributor

We don't require the output parameter to be present in get(), so you can just remove the option if there is not intent to support it.

This comment has been minimized.

Copy link
@ghost

ghost Jun 14, 2019

Author

I think option of output is probably helpful.so we can leave it until useless.


def get_capabilities(self):
result = dict()
result['rpc'] = self.get_base_rpc() + ['run_commands']

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 11, 2019

Contributor

You don't actually support all the functions included in get_base_rpc(), so it would be better to only specify the ones that you do support.

Suggested change
result['rpc'] = self.get_base_rpc() + ['run_commands']
result['rpc'] = ['get_capabilities', 'get', 'enable_response_logging', 'disable_response_logging', 'run_commands']
@@ -1790,4 +1790,18 @@ VERBOSE_TO_STDERR:
- section: defaults
key: verbose_to_stderr
type: bool
ERIC_ECCLI_ADDITIONAL_RE_FILE:

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 11, 2019

Contributor

This absolutely does not belong here. This should probably be moved to an option of the terminal plugin, though I am not sure why you need tunable regexes in the first place.

This comment has been minimized.

Copy link
@ghost

ghost Jun 13, 2019

Author

It will output some prompt information when running command through ECCLI model. In order to capture these custom prompt and we will add matching rule in configuration file instead of writing code in terminal plugin. I think it is more convenient and friendly for user of ansible, what do you think about?

@@ -27,6 +27,7 @@ Some Ansible Network platforms support multiple connection types, privilege esca
platform_slxos
platform_voss
platform_netconf_enabled
platform_eric_eccli

This comment has been minimized.

Copy link
@samccann

samccann Jun 11, 2019

Contributor

Please move this new platform up in the list. The list needs to be in alphabetical order.

You also need to add your new platform to the "Settings by Platform" table in this rst file.

name: Set the prompt and error information configuration file used in network eric eccli module
default: null
description:
- Specify a custom ECCLI prompt and error information configuration file,

This comment has been minimized.

Copy link
@samccann

samccann Jun 11, 2019

Contributor
Suggested change
- Specify a custom ECCLI prompt and error information configuration file,
- Specify a custom ECCLI prompt and error information configuration file.
default: null
description:
- Specify a custom ECCLI prompt and error information configuration file,
- For each command run through ECCLI module, performs additional checking,

This comment has been minimized.

Copy link
@samccann

samccann Jun 11, 2019

Contributor
Suggested change
- For each command run through ECCLI module, performs additional checking,
- For each command run through ECCLI module, perform additional checking.
- Specify a custom ECCLI prompt and error information configuration file,
- For each command run through ECCLI module, performs additional checking,
- If command's output matches any error regular expressions
- specified in this configuration file, then command runs fails.

This comment has been minimized.

Copy link
@samccann

samccann Jun 11, 2019

Contributor
Suggested change
- specified in this configuration file, then command runs fails.
- specified in this configuration file, then command run fails.
read from the device. This module includes an
argument that will cause the module to wait for a specific condition
before returning or timing out if the condition is not met.
- This module also support running commands in configuration mode

This comment has been minimized.

Copy link
@samccann

samccann Jun 11, 2019

Contributor
Suggested change
- This module also support running commands in configuration mode
- This module also supports running commands in configuration mode
cliconf: eccli
short_description: Use eccli cliconf to run command on Ericsson ECCLI platform
description:
- This eccli plugin provides low level abstraction apis for

This comment has been minimized.

Copy link
@samccann

samccann Jun 11, 2019

Contributor
Suggested change
- This eccli plugin provides low level abstraction apis for
- This eccli plugin provides low level abstraction APIs for

This comment has been minimized.

Copy link
@ghost

ghost Jun 14, 2019

Author

Thank you, I will correct it.

environment variable C(ANSIBLE_NET_AUTH_PASS) will be used instead.
notes:
- For more information on using Ansible to manage network devices see the :ref:`Ansible Network Guide <network_guide>`
- For more information on using Ansible to manage Ericsson devices see the Ericsson documents.

This comment has been minimized.

Copy link
@samccann

samccann Jun 11, 2019

Contributor
Suggested change
- For more information on using Ansible to manage Ericsson devices see the Ericsson documents.
- For more information on using Ansible to manage Ericsson devices see the Ericsson documentation.

This comment has been minimized.

Copy link
@ghost

ghost Jun 14, 2019

Author

Thank you, I will correct it.

ansible_ssh_common_args: '-o ProxyCommand="ssh -W %h:%p -q bastion01"'
- If you are using SSH keys (including an ssh-agent) you can remove the ``ansible_password`` configuration.

This comment has been minimized.

Copy link
@danielmellado

danielmellado Jun 12, 2019

Contributor

Aren't those just default ansible options? I do see no need of commenting they also here. Instead, I'd refer to the upstream ansible docs

- specified in this configuration file, then command runs fails.
- If command's output matches any prompt specified in this configuration file,
- the current received command's output is treated as complete.
env: [{name: ANSIBLE_ERIC_ECCLI_ADDITIONAL_RE_FILE}]

This comment has been minimized.

Copy link
@danielmellado

danielmellado Jun 12, 2019

Contributor

Please see Qalthos comment above

import os
import json

from units.modules.utils import AnsibleExitJson, AnsibleFailJson, ModuleTestCase

This comment has been minimized.

Copy link
@danielmellado

danielmellado Jun 12, 2019

Contributor

overall, ansible is moving to pytest, so it'd be great to have new modules use it for the testing.

@danielmellado
Copy link
Contributor

left a comment

Hi, I left some comments besides @Qalthos and @samccann , please address those and resubmit your changes. Thanks!

Cheng You
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/module_utils/network/eric_eccli/eric_eccli.py:33:11: undefined-variable Undefined variable 'eric_eccli_provider_spec'

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

lib/ansible/module_utils/network/eric_eccli/eric_eccli.py:60:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:200:4: E114 indentation is not a multiple of four (comment)
lib/ansible/plugins/terminal/eric_eccli.py:48:12: E111 indentation is not a multiple of four

click here for bot help

@ansible-zuul

This comment has been minimized.

Copy link

commented Jun 14, 2019

Build failed (third-party-check pipeline) integration testing with
Ansible.

Cheng You
@ghost
Copy link
Author

left a comment

Thank your comments. I will adopt these advise.

def get(self, command=None, prompt=None, answer=None, sendonly=False, output=None, check_all=False):
if not command:
raise ValueError('must provide value of command to execute')
if output:

This comment has been minimized.

Copy link
@ghost

ghost Jun 14, 2019

Author

I think option of output is probably helpful.so we can leave it until useless.

try:
config_file = C.ERIC_ECCLI_ADDITIONAL_RE_FILE
if config_file:
file = open(config_file, "r")

This comment has been minimized.

Copy link
@ghost

ghost Jun 14, 2019

Author

Thank you. I also agree your view.

new_pres = []
mode = 0
file = None
if platform.python_version().startswith('3'):

This comment has been minimized.

Copy link
@ghost

ghost Jun 14, 2019

Author

Thank you. I will adopt this import function instead of above.

environment variable C(ANSIBLE_NET_AUTH_PASS) will be used instead.
notes:
- For more information on using Ansible to manage network devices see the :ref:`Ansible Network Guide <network_guide>`
- For more information on using Ansible to manage Ericsson devices see the Ericsson documents.

This comment has been minimized.

Copy link
@ghost

ghost Jun 14, 2019

Author

Thank you, I will correct it.

cliconf: eccli
short_description: Use eccli cliconf to run command on Ericsson ECCLI platform
description:
- This eccli plugin provides low level abstraction apis for

This comment has been minimized.

Copy link
@ghost

ghost Jun 14, 2019

Author

Thank you, I will correct it.


_DEVICE_CONFIGS = {}

eric_eccli_provider_spec = {

This comment has been minimized.

Copy link
@ghost

ghost Jun 14, 2019

Author

Thank your advise, I will delete it.

}

eric_eccli_top_spec = {
'host': dict(removed_in_version=2.9),

This comment has been minimized.

Copy link
@ghost

ghost Jun 14, 2019

Author

Thank your advise. I will correct it.

@ansible-zuul

This comment has been minimized.

Copy link

commented Jun 14, 2019

@samdoran

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

Setting options to an empty string exposed a bug in our module validation code. I have a PR to fix that but until it is merged, here are sanity test failures:

ERROR: Found 3 validate-modules issue(s) which need to be resolved:
ERROR: lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: E305 DOCUMENTATION.author: Invalid author for dictionary value @ data['author']. Got 'Ericsson IPOS OAM team (@cheng.you@ericsson.com)'
ERROR: lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: E322 Argument 'authorize' is listed in the argument_spec, but not documented in the module documentation
ERROR: lib/ansible/modules/network/eric_eccli/eric_eccli_command.py:0:0: E331 Argument 'options' in argument_spec['provider'] must be a dictionary/hash when used
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@mattclay

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Closing since this PR was opened by a now deleted user.

@mattclay mattclay closed this Jun 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.