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

Host dev upstream #57086

Merged
merged 2 commits into from
Aug 6, 2019
Merged

Host dev upstream #57086

merged 2 commits into from
Aug 6, 2019

Conversation

ndswartz
Copy link
Contributor

@ndswartz ndswartz commented May 28, 2019 •
edited by samdoran
Loading

SUMMARY

Add host type strings for windows, windows cluster, linux and vmware to netapp_e_host module
Fix port removal and default group.
Fix port reassignment in netapp_e_host module.
Fix port iqn and wwpn case issue

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/modules/storage/netapp/netapp_e_host.py

ADDITIONAL INFORMATION
ansible 2.8.0.dev0
  config file = None
  configured module search path = [u'/home/swartzn/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/swartzn/ansible-dev/lib/ansible
  executable location = /home/swartzn/ansible-dev/bin/ansible
  python version = 2.7.15rc1 (default, Nov 12 2018, 14:31:15) [GCC 7.3.0]

@ansibot
Copy link
Contributor

ansibot commented May 28, 2019

cc @amit0701 @carchi8py @hulquest @lmprice @lonico @schmots1
click here for bot help

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 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. netapp storage support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels May 28, 2019
@goneri goneri removed the needs_triage Needs a first human triage before being processed. label May 30, 2019
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. test This PR relates to tests. and removed core_review In order to be merged, this PR must follow the core review workflow. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 7, 2019
@lmprice
Copy link
Contributor

lmprice commented Jul 10, 2019

I ran into one of these bugs today through general use. I'd like to see this one get merged.

@anshulbehl
Copy link
Contributor

@ndswartz can you take a look at the conflicts in this PR?

@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jul 30, 2019
@ansibot
Copy link
Contributor

ansibot commented Jul 30, 2019

@ndswartz this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot ansibot added merge_commit This PR contains at least one merge commit. Please resolve! and removed 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 Jul 30, 2019
@ansibot ansibot 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 Jul 30, 2019
@ndswartz
Copy link
Contributor Author

@anshulbehl Fixed the confict

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 30, 2019
@anshulbehl
Copy link
Contributor

@ndswartz I just checked the coverage for the file changes and it is just 50 percent. The methods valid_host_type, port_on_diff_host, create_host are not covered at all. Would it be possible for you to increase some coverage?

@swartzn
Improve netapp_e_host module
72ea3bb 
Add host type strings for windows, windows cluster, linux and vmware to netapp_e_host module
Make host port information case-insensitive in netapp_e_host module
Fix port removal and default group.
Fix port reassignment in netapp_e_host module.
Fix port label or address change within existing host object in module netapp_e_host
Add unit and integration tests
@ndswartz ndswartz force-pushed the host_dev_upstream branch 3 times, most recently from 3057957 to 9aec3bc Compare August 6, 2019 02:41
@ndswartz
Copy link
Contributor Author

ndswartz commented Aug 6, 2019

@anshulbehl I have updated and added the unit test. Thanks for point it out, they really need to be rewritten.

@anshulbehl
Copy link
Contributor

anshulbehl commented Aug 6, 2019 •
edited
Loading

/shipit

@anshulbehl anshulbehl merged commit acbffce into ansible:devel Aug 6, 2019
@ansible ansible locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@anshulbehl anshulbehl

Labels
affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. netapp storage support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ndswartz @ansibot @lmprice @anshulbehl @goneri @swartzn