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 nxos_gir issues #41809

Merged
merged 2 commits into from
Jun 28, 2018
Merged

fix nxos_gir issues #41809

merged 2 commits into from
Jun 28, 2018

Conversation

saichint
Copy link
Contributor

SUMMARY

fixes #41807

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

nxos_gir

ANSIBLE VERSION
ansible 2.7.0.dev0 (devel 8f144976df) last updated 2018/06/21 13:23:19 (GMT -400)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /root/agents-ci/ansible/lib/ansible
  executable location = /root/agents-ci/ansible/bin/ansible
  python version = 2.7.6 (default, Oct 26 2016, 20:30:19) [GCC 4.8.4]
ADDITIONAL INFORMATION
  • Fixes nxos_gir issues #41807
  • Currently integration test cases are all commented out. This is because putting device in maintenance mode causes connection issues. This PR only comments out the maintenance mode tasks and all other parameters are tested normally.
  • platform differences are addressed in test cases.

@ansibot
Copy link
Contributor

ansibot commented Jun 21, 2018

cc @GGabriele @mikewiebe @privateip @rahushen @rcarrillocruz @trishnaguha
click here for bot help

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 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:network This issue/PR relates to code supported by the Ansible Network Team. test This PR relates to tests. labels Jun 21, 2018
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Jun 21, 2018
trishnaguha
trishnaguha suggested changes Jun 27, 2018
View reviewed changes
lib/ansible/modules/network/nxos/nxos_gir.py Outdated
elif network_api == 'nxapi':
body = run_commands(module, cmds)

body = run_commands(module, cmds)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saichint Is execute_show_command even required? We can directly use run_commands since this method is calling run_commands only.

lib/ansible/modules/network/nxos/nxos_gir.py Outdated
elif module.params['system_mode_maintenance_shutdown'] is True:
elif (module.params[
'system_mode_maintenance_shutdown'] is True and
mode == 'normal'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about elif (module.params['system_mode_maintenance_shutdown'] and mode == 'normal'):?

lib/ansible/modules/network/nxos/nxos_gir.py
commands.append('system mode maintenance shutdown')
elif (module.params[
'system_mode_maintenance_shutdown'] is False and
mode == 'maintenance'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about elif not module.params['system_mode_maintenance_shutdown'] and mode == 'maintenance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot be done because system_mode_maintenance_shutdown is a boolean. If this parameter is completely omitted in the playbook it will be None and this condition will still be satisfied if we do the way you suggested. So I am leaving it as it.

test/integration/targets/nxos_gir/tests/common/sanity.yaml
- block:
- name: "Put system in maintenance mode with reload reset reason"
nxos_gir: &reset_reason
system_mode_maintenance_on_reload_reset_reason: manual_reload
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add provider: {{ connection }} also in the testcases as we are restoring nxapi tests #41818

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added provider

@trishnaguha trishnaguha self-assigned this Jun 27, 2018
@ansibot 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. labels Jun 27, 2018
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jun 27, 2018
@saichint
Copy link
Contributor Author

@trishnaguha Made changes as requested except one and I put my comments for that. Let me know if this is fine

@saichint
Copy link
Contributor Author

Due to stale_ci this is pr is being closed and will reopen soon

@saichint saichint closed this Jun 27, 2018
@saichint
Copy link
Contributor Author

reopening for needs_ci

@saichint saichint reopened this Jun 27, 2018
@ansibot ansibot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jun 27, 2018
@trishnaguha trishnaguha merged commit bd9b8b4 into ansible:devel Jun 28, 2018
@trishnaguha trishnaguha moved this from In Review to Need CP into 2.6 in zzz NOT USED: Networking Bugs Jun 28, 2018
@trishnaguha trishnaguha moved this from Need CP into 2.6 to Done in zzz NOT USED: Networking Bugs Jul 4, 2018
@saichint saichint deleted the gir branch July 20, 2018 18:35
@dagwieers dagwieers added the cisco Cisco technologies label Feb 23, 2019
@ansible ansible locked and limited conversation to collaborators Jul 1, 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

Assignees

@trishnaguha trishnaguha

Labels
affects_2.7 This issue/PR affects Ansible v2.7 bug This issue/PR relates to a bug. cisco Cisco technologies module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. networking Network category nxos Cisco NXOS 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.

nxos_gir issues
5 participants
@saichint @ansibot @trishnaguha @dagwieers @samdoran