network.py:ActionModule:run: does not honor _handle_src_option failures #52745

chrisvanheuveln · 2019-02-21T20:08:23Z

SUMMARY

PR #50301 moved template error handling out of run() and into its own method in handle_src_option; however, after the change run() ignores the return value so any errors are ignored.

My fix looks for error state and returns it if present. Verified fix with nxos_config/tests/common/src_* tests.

Ref:
71113ee#diff-7477bf046013758366cc85b06f90709aR43

Also updated the src_basic.yaml test which didn't actually test with src: before.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

plugins/action/network

ADDITIONAL INFORMATION

Reproduceable with nxos_config/tests/common/src_invalid.yaml. The symptom is that the src: string fails the template handling (as expected) but the string is sent to the device anyway.

From src_invalid.yaml:

- name: configure with invalid src
  nxos_config:
    src: basic/foobar.j2
    provider: "{{ connection }}"
  register: result
  ignore_errors: yes

Test output:

    "msg": "basic/foobar.j2\r\r\n                   ^\r\n% Invalid command at '^' marker.\r\n\rn9k-109(config)# "
...
fatal: [n9k-109.cisco.com]: FAILED! => {
    "assertion": "result.msg == 'path specified in src not found'",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"

chrisvanheuveln · 2019-02-21T20:18:49Z

@mikewiebe

ansibot · 2019-02-21T20:30:57Z

cc @NilashishC @Qalthos @ganeshrn @gdpak @justjais @privateip @rahushen @rcarrillocruz @trishnaguha
click here for bot help

ganeshrn

The issue is similar to #52911 which is fixed by PR #52912
However, the changes in integration test are still valid. Can you please revert the changes to file plugins/action/network.py as it not required after PR #52912 is merged

ansibot · 2019-02-27T20:55:33Z

@chrisvanheuveln this PR contains the following merge commits:

c5c72f3

Please rebase your branch to remove these commits.

click here for bot help

ansibot · 2019-02-27T21:53:38Z

@chrisvanheuveln this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

PR ansible#50301 moved template error handling out of run() and into its own method in `_handle_src_option`; however, after the change run() ignores the return value so any errors are ignored. Reproduceable with `nxos_config/tests/common/src_invalid.yaml` Verified fix with `nxos_config/tests/common/src_*` tests. Ref: ansible@71113ee#diff-7477bf046013758366cc85b06f90709aR43

This test was not actually testing with `src:` as it should have.

PR ansible#52912 fixed this already.

chrisvanheuveln · 2019-02-27T22:02:04Z

Can you please revert the changes to file plugins/action/network.py as it not required after PR #52912 is merged

@ganeshrn The file has been reverted.

ganeshrn

@chrisvanheuveln Thank you!

…es (ansible#52745) * network.py:ActionModule:run: does not honor _handle_src_option failures PR ansible#50301 moved template error handling out of run() and into its own method in `_handle_src_option`; however, after the change run() ignores the return value so any errors are ignored. Reproduceable with `nxos_config/tests/common/src_invalid.yaml` Verified fix with `nxos_config/tests/common/src_*` tests. Ref: ansible@71113ee#diff-7477bf046013758366cc85b06f90709aR43 * nxos_config/tests/common/src_basic: Updated to test with src This test was not actually testing with `src:` as it should have. * Revert 412d7e change to plugins/action/network.py PR ansible#52912 fixed this already. * nxos_config: fix src_invalid test (cherry picked from commit d69239c)

…es (#52745) * network.py:ActionModule:run: does not honor _handle_src_option failures PR #50301 moved template error handling out of run() and into its own method in `_handle_src_option`; however, after the change run() ignores the return value so any errors are ignored. Reproduceable with `nxos_config/tests/common/src_invalid.yaml` Verified fix with `nxos_config/tests/common/src_*` tests. Ref: 71113ee#diff-7477bf046013758366cc85b06f90709aR43 * nxos_config/tests/common/src_basic: Updated to test with src This test was not actually testing with `src:` as it should have. * Revert 412d7e change to plugins/action/network.py PR #52912 fixed this already. * nxos_config: fix src_invalid test (cherry picked from commit d69239c)

chrisvanheuveln changed the title ~~template handler _handle_src_option does not honor failures~~ etwork.py:ActionModule:run: does not honor _handle_src_option failures Feb 21, 2019

chrisvanheuveln changed the title ~~etwork.py:ActionModule:run: does not honor _handle_src_option failures~~ network.py:ActionModule:run: does not honor _handle_src_option failures Feb 21, 2019

dagwieers added the cisco Cisco technologies label Feb 22, 2019

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_triage Needs a first human triage before being processed. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 22, 2019

NilashishC requested a review from ganeshrn February 27, 2019 15:08

ganeshrn suggested changes Feb 27, 2019

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. new_contributor This PR is the first contribution by a new community member. labels Feb 27, 2019

ansibot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed small_patch labels Feb 27, 2019

chrisvanheuveln added 4 commits February 27, 2019 16:59

nxos_config/tests/common/src_basic: Updated to test with src

ee42582

This test was not actually testing with `src:` as it should have.

Revert 412d7e change to plugins/action/network.py

a995dcc

PR ansible#52912 fixed this already.

nxos_config: fix src_invalid test

183a860

chrisvanheuveln force-pushed the devel-src_invalid branch from 978525b to 183a860 Compare February 27, 2019 22:00

ansibot added small_patch and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Feb 27, 2019

ganeshrn approved these changes Feb 28, 2019

View reviewed changes

ganeshrn merged commit d69239c into ansible:devel Feb 28, 2019

chrisvanheuveln deleted the devel-src_invalid branch March 8, 2019 16:28

ansible locked and limited conversation to collaborators Jul 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network.py:ActionModule:run: does not honor _handle_src_option failures #52745

network.py:ActionModule:run: does not honor _handle_src_option failures #52745

chrisvanheuveln commented Feb 21, 2019

chrisvanheuveln commented Feb 21, 2019

ansibot commented Feb 21, 2019

ganeshrn left a comment

ansibot commented Feb 27, 2019

ansibot commented Feb 27, 2019

chrisvanheuveln commented Feb 27, 2019

ganeshrn left a comment

network.py:ActionModule:run: does not honor _handle_src_option failures #52745

network.py:ActionModule:run: does not honor _handle_src_option failures #52745

Conversation

chrisvanheuveln commented Feb 21, 2019

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

chrisvanheuveln commented Feb 21, 2019

ansibot commented Feb 21, 2019

ganeshrn left a comment

Choose a reason for hiding this comment

ansibot commented Feb 27, 2019

ansibot commented Feb 27, 2019

chrisvanheuveln commented Feb 27, 2019

ganeshrn left a comment

Choose a reason for hiding this comment