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

Bug Fix #53271 (return commands for cli_config) #58766

Open
wants to merge 3 commits into
base: devel
from

Conversation

@adharshsrivatsr
Copy link

commented Jul 5, 2019

SUMMARY

Added fix to include the pushed commands in the results dictionary for the cli_config module.

Fixes #53271

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

cli_config

ADDITIONAL INFORMATION

The commands pushed to a device were not earlier a part of the results dictionary, but they should have been included as mentioned in the documentation of cli_config. This fix will resolve it.

ok: [localhost] => {
    "res": {
        "ansible_facts": {
            "discovered_interpreter_python": "/usr/bin/python"
        }, 
        "changed": true, 
        "commands": [
            "hostname New_Device", 
            "vlan 7", 
            "name New_VLAN"
        ], 
        "deprecations": [
            {
                "msg": "Distribution fedora 30 on host localhost should use /usr/bin/python3, but is using /usr/bin/python for backward compatibility with prior Ansible releases. A future Ansible release will default to using the discovered platform python for this host. See https://docs.ansible.com/ansible/devel/reference_appendices/interpreter_discovery.html for more information", 
                "version": "2.12"
            }
        ], 
        "failed": false
    }
}

adharshsrivatsr added some commits Jul 5, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

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

lib/ansible/modules/network/cli/cli_config.py:237:0: trailing-whitespace Trailing whitespace
lib/ansible/modules/network/cli/cli_config.py:293:63: trailing-whitespace Trailing whitespace
lib/ansible/modules/network/cli/cli_config.py:384:0: trailing-whitespace Trailing whitespace
lib/ansible/modules/network/cli/cli_config.py:385:0: trailing-whitespace Trailing whitespace

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

lib/ansible/modules/network/cli/cli_config.py:237:1: W293 blank line contains whitespace
lib/ansible/modules/network/cli/cli_config.py:293:64: W291 trailing whitespace
lib/ansible/modules/network/cli/cli_config.py:384:1: W293 blank line contains whitespace
lib/ansible/modules/network/cli/cli_config.py:385:1: W293 blank line contains whitespace

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

@adharshsrivatsr, 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

@@ -290,6 +290,7 @@ def run(module, device_operations, connection, candidate, running, rollback_id):
if commit:
connection.edit_config(**kwargs)
result['changed'] = True
result['commands'] = (str(config_diff).split('\n'))

This comment has been minimized.

Copy link
@NilashishC

NilashishC Jul 5, 2019

Contributor

With this, the commands key won't be returned for platforms that support on box diff. This needs to be handled in supports_onbox_diff block too.

However, on box diff, usually doesn't return commands, instead, it returns something like:

edit interfaces ethernet eth1]
+description "Configured by Ansible - Interface 1"
+mtu 1500
+vif 100 {
+    description "Eth1 - VIF 100"
+    mtu 400
+}
+vif 101 {
+    description "Eth1 - VIF 101"
+}
[edit interfaces]
+ethernet eth2 {
+    description "Configured by Ansible - Interface 2 (ADMIN DOWN)"
+    disable
+    duplex auto
+    mtu 600
+    smp_affinity auto
+    speed auto
+}
[edit]

@adharshsrivatsr adharshsrivatsr force-pushed the adharshsrivatsr:devel branch from 7e6e07d to c5ad89b Jul 11, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

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

lib/ansible/modules/network/cli/cli_config.py:236:0: trailing-whitespace Trailing whitespace
lib/ansible/modules/network/cli/cli_config.py:292:60: trailing-whitespace Trailing whitespace
lib/ansible/modules/network/cli/cli_config.py:303:0: trailing-whitespace Trailing whitespace
lib/ansible/modules/network/cli/cli_config.py:383:0: trailing-whitespace Trailing whitespace
lib/ansible/modules/network/cli/cli_config.py:384:0: trailing-whitespace Trailing whitespace

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

lib/ansible/modules/network/cli/cli_config.py:186:161: E501 line too long (292 > 160 characters)
lib/ansible/modules/network/cli/cli_config.py:186:293: W291 trailing whitespace
lib/ansible/modules/network/cli/cli_config.py:236:1: W293 blank line contains whitespace
lib/ansible/modules/network/cli/cli_config.py:292:61: W291 trailing whitespace
lib/ansible/modules/network/cli/cli_config.py:303:1: W293 blank line contains whitespace
lib/ansible/modules/network/cli/cli_config.py:383:1: W293 blank line contains whitespace
lib/ansible/modules/network/cli/cli_config.py:384:1: W293 blank line contains whitespace

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

lib/ansible/modules/network/cli/cli_config.py:186:82: E313 RETURN is not valid YAML

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

lib/ansible/modules/network/cli/cli_config.py:186:82: error RETURN: syntax error: mapping values are not allowed here

click here for bot help

@ansibot ansibot added the ci_verified label Jul 11, 2019

@pabelanger
Copy link
Contributor

left a comment

we also should include a test here, to validate new functionality

@pabelanger

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

recheck

@ansible-zuul

This comment has been minimized.

Copy link

commented Jul 12, 2019

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

@trishnaguha trishnaguha self-assigned this Jul 17, 2019

@@ -184,7 +183,7 @@

RETURN = """
commands:
description: The set of commands that will be pushed to the remote device
description: The set of commands that will be pushed to the remote device. Note: The commands will be returned only for platforms that have C(supports_onbox_diff) set to I(false). For other platforms, using the C(--diff) option with the playbook will return the difference in configuration.

This comment has been minimized.

Copy link
@trishnaguha

trishnaguha Jul 17, 2019

Member

The Note section should be on the next line which follows the indentation of description section.

This comment has been minimized.

Copy link
@adharshsrivatsr

adharshsrivatsr Jul 17, 2019

Author

Should it be 'Note:' or 'note:' ? Because all section keys are lower cased. Will this affect how the template is processed?

@@ -184,7 +183,7 @@

RETURN = """
commands:
description: The set of commands that will be pushed to the remote device
description: The set of commands that will be pushed to the remote device. Note: The commands will be returned only for platforms that have C(supports_onbox_diff) set to I(false). For other platforms, using the C(--diff) option with the playbook will return the difference in configuration.

This comment has been minimized.

Copy link
@trishnaguha

trishnaguha Jul 17, 2019

Member

The sentence can be rephrased to The commands will be returned only for platforms that does not support onbox diff. The C(--diff) option with the playbook will return the difference in configuration for devices that has support for onbox diff.

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.