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

VMware: Fix module usages in module_utils #49421

Merged
merged 4 commits into from Dec 4, 2018
Merged

VMware: Fix module usages in module_utils #49421

merged 4 commits into from Dec 4, 2018

Conversation

Akasurde
Copy link
Member

@Akasurde Akasurde commented Dec 2, 2018

SUMMARY

Added unit tests for vmware.py

Signed-off-by: Abhijeet Kasurde akasurde@redhat.com

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

changelogs/fragments/47313-vmware-fix_module_error.yaml
lib/ansible/module_utils/vmware.py
test/units/module_utils/test_vmware.py

@ansibot
Copy link
Contributor

ansibot commented Dec 2, 2018

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

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Dec 2, 2018

cc @ckotte @dav1x @warthog9
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. needs_triage Needs a first human triage before being processed. 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. vmware VMware community 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 Dec 2, 2018
@Akasurde
VMware: Fix module usages in module_utils
3c5b210 
Added unit tests for vmware.py

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@Akasurde
Copy link
Member Author

Akasurde commented Dec 2, 2018

@mattclay Not sure what is happening here. But on my setup, test is passing -

[gw3] [ 50%] PASSED test/units/module_utils/test_vmware.py::TestVmware::test_vmware_missing_ensure_libs
[gw5] [100%] PASSED test/units/module_utils/test_vmware.py::TestVmware::test_vmware_found_ensure_libs

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 2, 2018
resmo
resmo approved these changes Dec 2, 2018
View reviewed changes
Copy link
Contributor

@resmo resmo left a comment

Choose a reason for hiding this comment

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

shipit

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. labels Dec 2, 2018
mattclay
mattclay requested changes Dec 3, 2018
View reviewed changes
test/units/module_utils/test_vmware.py Outdated Show resolved Hide resolved
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed shipit This PR is ready to be merged by Core labels Dec 3, 2018
@Akasurde
Single test for both missing and found case
4a77334 
Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@Akasurde
Skip test for Python 2.6
c2c20e5 
Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
pdellaert
pdellaert approved these changes Dec 3, 2018
View reviewed changes
Copy link
Contributor

@pdellaert pdellaert left a comment

Choose a reason for hiding this comment

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

Do you want to have the unit tests in this PR? It looks like a good start and i don't mind. But the changelog doesn't reflect it :)

@Akasurde
Copy link
Member Author

Akasurde commented Dec 3, 2018

@pdellaert I added TCs for this change only. Would love to see them backported to 2.7 and 2.6.

@Akasurde
Review comments
bb2d518 
Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@Akasurde
Copy link
Member Author

Akasurde commented Dec 4, 2018

@mattclay Could please review this ? Thanks.

abadger
abadger reviewed Dec 4, 2018
View reviewed changes
test/units/module_utils/test_vmware.py
raise ExitJson(*args, **kwargs)

def fail_json(self, *args, **kwargs):
raise FailJson(*args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This is not a blocker:

I haven't tested but you probably don't have to do this. You can catch SystemExit instead.

This does give you the additional data that exit_json vs fail_json was called, though. (although since you're checking the exit message, that might be redundant.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 4, 2018
@Akasurde Akasurde merged commit 1b04571 into ansible:devel Dec 4, 2018
@Akasurde
Copy link
Member Author

Akasurde commented Dec 4, 2018

@resmo @abadger @mattclay @pdellaert Thanks for the review.

@Akasurde Akasurde deleted the i47313_followup branch December 4, 2018 04:42
Akasurde added a commit to Akasurde/ansible that referenced this pull request Dec 4, 2018
@Akasurde
VMware: Fix module usages in module_utils (ansible#49421)
dc90c1d 
* VMware: Fix module usages in module_utils
* Skip test for Python 2.6 as SSL context is not available in Python 2.6

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
(cherry picked from commit 1b04571)
Akasurde added a commit to Akasurde/ansible that referenced this pull request Dec 4, 2018
@Akasurde
VMware: Fix module usages in module_utils (ansible#49421)
0fb970a 
* VMware: Fix module usages in module_utils
* Skip test for Python 2.6 as SSL context is not available in Python 2.6

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
(cherry picked from commit 1b04571)
abadger pushed a commit that referenced this pull request Dec 4, 2018
@Akasurde @abadger
VMware: Fix module usages in module_utils (#49421)
35d84cc 
* VMware: Fix module usages in module_utils
* Skip test for Python 2.6 as SSL context is not available in Python 2.6

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
(cherry picked from commit 1b04571)
@Akasurde Akasurde mentioned this pull request Dec 5, 2018
Closed
mattclay pushed a commit that referenced this pull request Dec 10, 2018
@Akasurde @mattclay
VMware: Fix module usages in module_utils (#49421)
5a7b01f 
* VMware: Fix module usages in module_utils
* Skip test for Python 2.6 as SSL context is not available in Python 2.6

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
(cherry picked from commit 1b04571)
kbreit pushed a commit to kbreit/ansible that referenced this pull request Jan 11, 2019
@Akasurde @kbreit
VMware: Fix module usages in module_utils (ansible#49421)
6ed0f9f 
* VMware: Fix module usages in module_utils
* Skip test for Python 2.6 as SSL context is not available in Python 2.6

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@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

@abadger abadger abadger left review comments

@resmo resmo resmo approved these changes

@mattclay mattclay mattclay approved these changes

@pdellaert pdellaert pdellaert approved these changes

Assignees
No one assigned
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. cloud core_review In order to be merged, this PR must follow the core review workflow. 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. vmware VMware community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Akasurde @ansibot @resmo @abadger @mattclay @pdellaert @dagwieers