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.

Sign up for GitHub

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

Jump to bottom

Fix issues with nxos_os_install module #48811

Merged
merged 11 commits into from
Nov 20, 2018
Merged

Fix issues with nxos_os_install module #48811

merged 11 commits into from
Nov 20, 2018

Conversation

mikewiebe
Copy link
Contributor

SUMMARY
  • Fixes an issue in module_utils/network/nxos/nxos.py where only the code -32603 was returned by the load_config function for network_cli in cases where an invalid command is sent or the connection times out.
    • We need more granularity so instead of returning the code, load_config now returns the actual error so upstream consumers have more data to process.
    • This was causing false positives for upgrades using nxos_install_os
  • Also fixes an issue with the force keyword in the nxos_install_os module as it's not supported on all platforms. The fix to module_utils/network/nxos/nxos.py was needed for this fix.
  • Adds additional integration tests to make it easer to test the various platforms.
    • Removes redundancy in the tests
    • Makes use of the nxos_file_copy module in the tests.
  • Small change to nxos_file_copy to make the code more readable for copies initiated from the nxos device.
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

nxos_install_os

ADDITIONAL INFORMATION

Changes tested against:

  • N9k (disruptive and non-disruptive upgrades)
  • N3500 and N3172 platforms
  • N7k

@ansibot
Copy link
Contributor

ansibot commented Nov 16, 2018

Hi @mikewiebe, thank you for submitting this pull-request!

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Nov 16, 2018

cc @rahushen @rcarrillocruz @trishnaguha @tstoner
click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. networking Network category nxos Cisco NXOS community support:community This issue/PR relates to code supported by the Ansible community. support:network This issue/PR relates to code supported by the Ansible Network Team. test This PR relates to tests. labels Nov 16, 2018
@ansibot
Copy link
Contributor

ansibot commented Nov 16, 2018

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

lib/ansible/modules/network/nxos/nxos_install_os.py:583:14: ansible-format-automatic-specification Format string contains automatic field numbering specification

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

test/integration/targets/nxos_install_os/tests/common/upgrade.yaml:29:2: error syntax error: expected <block end>, but found '<block mapping start>'

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. 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. labels Nov 16, 2018
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 16, 2018
@trishnaguha trishnaguha self-assigned this Nov 19, 2018
@trishnaguha trishnaguha removed the needs_triage Needs a first human triage before being processed. label Nov 19, 2018
@trishnaguha
Copy link
Member

Thanks for the PR @mikewiebe

@trishnaguha trishnaguha merged commit a721572 into ansible:devel Nov 20, 2018
@trishnaguha trishnaguha added this to Need CP to into 2.7 in zzz NOT USED: Networking Bugs Nov 20, 2018
@mikewiebe mikewiebe deleted the rel280/fix_nxos_install_os branch November 26, 2018 18:05
trishnaguha pushed a commit to trishnaguha/ansible that referenced this pull request Nov 29, 2018
@mikewiebe @trishnaguha
Fix issues with nxos_os_install module (ansible#48811)
6c7325c 
* Use expect module to copy files

* Remove old and redundant upgrade files

* Return error message instead of code

* Cleanup copy command code

* Fix force issue in nxos_install_os

* new nxos_install_os integration tests

* Uncomment transport tests

* Revert negative test change

* Remove combined option that is no longer required

* Make shippable happy

* Add n5k test files

(cherry picked from commit a721572)
@trishnaguha trishnaguha moved this from Need CP to into 2.7 to Need CP into 2.6 in zzz NOT USED: Networking Bugs Nov 29, 2018
@trishnaguha trishnaguha moved this from Need CP into 2.6 to Done in zzz NOT USED: Networking Bugs Nov 29, 2018
trishnaguha pushed a commit to trishnaguha/ansible that referenced this pull request Nov 29, 2018
@mikewiebe @trishnaguha
Fix issues with nxos_os_install module (ansible#48811)
9b40d1e 
* Use expect module to copy files

* Remove old and redundant upgrade files

* Return error message instead of code

* Cleanup copy command code

* Fix force issue in nxos_install_os

* new nxos_install_os integration tests

* Uncomment transport tests

* Revert negative test change

* Remove combined option that is no longer required

* Make shippable happy

* Add n5k test files

(cherry picked from commit a721572)
mjmayer pushed a commit to mjmayer/ansible that referenced this pull request Nov 30, 2018
@mikewiebe @mjmayer
Fix issues with nxos_os_install module (ansible#48811)
ead1405 
* Use expect module to copy files

* Remove old and redundant upgrade files

* Return error message instead of code

* Cleanup copy command code

* Fix force issue in nxos_install_os

* new nxos_install_os integration tests

* Uncomment transport tests

* Revert negative test change

* Remove combined option that is no longer required

* Make shippable happy

* Add n5k test files
abadger pushed a commit that referenced this pull request Dec 4, 2018
@mikewiebe @abadger
Fix issues with nxos_os_install module (#48811)
d1a38f1 
* Use expect module to copy files

* Remove old and redundant upgrade files

* Return error message instead of code

* Cleanup copy command code

* Fix force issue in nxos_install_os

* new nxos_install_os integration tests

* Uncomment transport tests

* Revert negative test change

* Remove combined option that is no longer required

* Make shippable happy

* Add n5k test files

(cherry picked from commit a721572)
Tomorrow9 pushed a commit to Tomorrow9/ansible that referenced this pull request Dec 4, 2018
@mikewiebe @Tomorrow9
Fix issues with nxos_os_install module (ansible#48811)
4894aff 
* Use expect module to copy files

* Remove old and redundant upgrade files

* Return error message instead of code

* Cleanup copy command code

* Fix force issue in nxos_install_os

* new nxos_install_os integration tests

* Uncomment transport tests

* Revert negative test change

* Remove combined option that is no longer required

* Make shippable happy

* Add n5k test files
@mikewiebe mikewiebe mentioned this pull request Dec 5, 2018
Closed
mattclay pushed a commit that referenced this pull request Dec 10, 2018
@mikewiebe @mattclay
Fix issues with nxos_os_install module (#48811)
e3c4e49 
* Use expect module to copy files

* Remove old and redundant upgrade files

* Return error message instead of code

* Cleanup copy command code

* Fix force issue in nxos_install_os

* new nxos_install_os integration tests

* Uncomment transport tests

* Revert negative test change

* Remove combined option that is no longer required

* Make shippable happy

* Add n5k test files

(cherry picked from commit a721572)
@dagwieers dagwieers added the cisco Cisco technologies label Feb 23, 2019
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@trishnaguha trishnaguha trishnaguha approved these changes

@Qalthos Qalthos Awaiting requested review from Qalthos

Assignees

@trishnaguha trishnaguha

Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. cisco Cisco technologies core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. networking Network category nxos Cisco NXOS community support:community This issue/PR relates to code supported by the Ansible community. support:network This issue/PR relates to code supported by the Ansible Network Team. test This PR relates to tests.
Projects
No open projects
zzz NOT USED: Networking Bugs
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mikewiebe @ansibot @trishnaguha @dagwieers