vmware / vsphere-automation-sdk-python

Python samples, language bindings, and API reference documentation for vSphere, VMC, and NSX-T using the VMware REST API
MIT License
748 stars 312 forks source link

Remove suds(-jurko) dependency #287

Closed mariolenz closed 3 years ago

mariolenz commented 3 years ago

It looks like you don't really need suds or suds-jurko. grep -R suds lib tests doesn't find anything...

Fixes #286

vmwclabot commented 3 years ago

@mariolenz, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

shwetapurohit commented 3 years ago

@mariolenz did you test the PR? if so, please share the output after removing "suds" dependency

mariolenz commented 3 years ago

did you test the PR?

@shwetapurohit Not much. I've created a venv, installed ansible and this SDK without suds / suds-jurko and used vmware_guest_info from community.vmware to test the change. This module uses the SDK to get the tags of a VM.

But this probably didn't cover much code. So I've asked the others who ran into this issue to test this, too. Just to make sure this change really doesn't break anything.

please share the output after removing "suds" dependency

What output? The ansible module I used to test this worked fine, that's all the output I hoped for ;-)

Or do you want me to run pytest? This gives the following result:

(venv-ansible-vmware) [vsphere-automation-sdk-python-master]$ pytest
================================================== test session starts ===================================================
platform linux -- Python 3.6.8, pytest-6.2.5, py-1.10.0, pluggy-1.0.0
rootdir: /home/mario/vsphere-automation-sdk-python-master
collected 29 items

tests/test_vmc_client.py ...........                                                                               [ 37%]
tests/test_vsphere_client.py ..................                                                                    [100%]

==================================================== warnings summary ====================================================
<unknown>:1
<unknown>:1
  <unknown>:1: DeprecationWarning: invalid escape sequence \[

-- Docs: https://docs.pytest.org/en/stable/warnings.html
============================================= 29 passed, 2 warnings in 0.57s =============================================
(venv-ansible-vmware) [vsphere-automation-sdk-python-master]$
mariolenz commented 3 years ago

@shwetapurohit Looks like this works in our CI pipeline: ansible-collections/community.vmware#1041

I'd say we have quite a good coverage when it comes to our modules. But I don't know how much of your code is really tested by us. It's possible that we only use a very small part of your SDK in our modules.

Anyway, I hope this information helps you at least a bit.

mariolenz commented 3 years ago

Here is another run of our CI pipeline, but running more tests: ansible-collections/community.vmware#1042

But as I've already said, it's possible (actually, it's even probable) that our tests cover only a fraction of your code.

mariolenz commented 3 years ago

@shwetapurohit Even when I install your code from my branch...

root [ / ]# python3 -m venv /tmp/pr287
root [ / ]# source /tmp/pr287/bin/activate
(pr287) root [ / ]# pip3 install git+https://github.com/mariolenz/vsphere-automation-sdk-python.git@remove_suds-jurko_dependency
Collecting git+https://github.com/mariolenz/vsphere-automation-sdk-python.git@remove_suds-jurko_dependency
  Cloning https://github.com/mariolenz/vsphere-automation-sdk-python.git (to revision remove_suds-jurko_dependency) to /tmp/pip-req-build-90h121xk
  Running command git clone -q https://github.com/mariolenz/vsphere-automation-sdk-python.git /tmp/pip-req-build-90h121xk
  Running command git checkout -b remove_suds-jurko_dependency --track origin/remove_suds-jurko_dependency
  Switched to a new branch 'remove_suds-jurko_dependency'
  Branch 'remove_suds-jurko_dependency' set up to track remote branch 'remove_suds-jurko_dependency' from 'origin'.
  Resolved https://github.com/mariolenz/vsphere-automation-sdk-python.git to commit 0457540d284c53e5f16a7a709d80145f21dc9166
Processing ./tmp/pip-req-build-90h121xk/lib/vapi-runtime/vapi_runtime-2.25.0-py2.py3-none-any.whl
Processing ./tmp/pip-req-build-90h121xk/lib/vapi-client-bindings/vapi_client_bindings-3.6.0-py2.py3-none-any.whl
Processing ./tmp/pip-req-build-90h121xk/lib/vapi-common-client/vapi_common_client-2.25.0-py2.py3-none-any.whl
Processing ./tmp/pip-req-build-90h121xk/lib/vmc-client-bindings/vmc_client_bindings-1.52.0-py2.py3-none-any.whl
Processing ./tmp/pip-req-build-90h121xk/lib/nsx-python-sdk/nsx_python_sdk-3.1.2.1.1-py2.py3-none-any.whl
Processing ./tmp/pip-req-build-90h121xk/lib/nsx-policy-python-sdk/nsx_policy_python_sdk-3.1.2.1.1-py2.py3-none-any.whl
Processing ./tmp/pip-req-build-90h121xk/lib/nsx-vmc-policy-python-sdk/nsx_vmc_policy_python_sdk-3.1.2.1.1-py2.py3-none-any.whl
Processing ./tmp/pip-req-build-90h121xk/lib/nsx-vmc-aws-integration-python-sdk/nsx_vmc_aws_integration_python_sdk-3.1.2.1.1-py2.py3-none-any.whl
Processing ./tmp/pip-req-build-90h121xk/lib/vmc-draas-client-bindings/vmc_draas_client_bindings-1.18.0-py2.py3-none-any.whl
Collecting lxml>=4.3.0
  Downloading lxml-4.6.3-cp39-cp39-manylinux2014_x86_64.whl (6.9 MB)
     |████████████████████████████████| 6.9 MB 9.2 MB/s 
Collecting pyVmomi>=6.7
  Downloading pyvmomi-7.0.2.tar.gz (589 kB)
     |████████████████████████████████| 589 kB 16.7 MB/s 
Collecting requests>=2.3.0
  Downloading requests-2.26.0-py2.py3-none-any.whl (62 kB)
     |████████████████████████████████| 62 kB 813 kB/s 
Collecting six>=1.7.3
  Downloading six-1.16.0-py2.py3-none-any.whl (11 kB)
Collecting idna<4,>=2.5
  Downloading idna-3.2-py3-none-any.whl (59 kB)
     |████████████████████████████████| 59 kB 4.3 MB/s 
Collecting charset-normalizer~=2.0.0
  Downloading charset_normalizer-2.0.6-py3-none-any.whl (37 kB)
Collecting certifi>=2017.4.17
  Downloading certifi-2021.5.30-py2.py3-none-any.whl (145 kB)
     |████████████████████████████████| 145 kB 18.4 MB/s 
Collecting urllib3<1.27,>=1.21.1
  Downloading urllib3-1.26.6-py2.py3-none-any.whl (138 kB)
     |████████████████████████████████| 138 kB 6.2 MB/s 
Requirement already satisfied: setuptools in /tmp/pr287/lib/python3.9/site-packages (from vapi-runtime@ file://localhost//tmp/pip-req-build-90h121xk/lib/vapi-runtime/vapi_runtime-2.25.0-py2.py3-none-any.whl->vSphere-Automation-SDK==1.69.0) (49.2.1)
Collecting pyOpenSSL>=18.0.0
  Downloading pyOpenSSL-20.0.1-py2.py3-none-any.whl (54 kB)
     |████████████████████████████████| 54 kB 3.6 MB/s 
Collecting cryptography>=3.2
  Downloading cryptography-3.4.8-cp36-abi3-manylinux_2_24_x86_64.whl (3.0 MB)
     |████████████████████████████████| 3.0 MB 13.0 MB/s 
Collecting cffi>=1.12
  Downloading cffi-1.14.6-cp39-cp39-manylinux1_x86_64.whl (405 kB)
     |████████████████████████████████| 405 kB 7.7 MB/s 
Collecting pycparser
  Downloading pycparser-2.20-py2.py3-none-any.whl (112 kB)
     |████████████████████████████████| 112 kB 6.9 MB/s 
Building wheels for collected packages: vSphere-Automation-SDK, pyVmomi
  Building wheel for vSphere-Automation-SDK (setup.py) ... done
  Created wheel for vSphere-Automation-SDK: filename=vSphere_Automation_SDK-1.69.0-py3-none-any.whl size=2324 sha256=3f1c42667ed57a7843b431f863fefd1f6703504890329477c71abf9a3425cd2b
  Stored in directory: /tmp/pip-ephem-wheel-cache-t8qs2xpj/wheels/b7/e7/99/8a504242d6e72f10ab618482389065efca5f7be96fbb194f4a
  Building wheel for pyVmomi (setup.py) ... done
  Created wheel for pyVmomi: filename=pyvmomi-7.0.2-py2.py3-none-any.whl size=256259 sha256=135822ff8a718c99be8622f1cba80e46a2fa5373c975c9ca6f711f9f4cf79312
  Stored in directory: /root/.cache/pip/wheels/d4/26/c7/fcba7d3abca45dae97279d2a8ee5084d69a85a16cc629af9eb
Successfully built vSphere-Automation-SDK pyVmomi
Installing collected packages: pycparser, cffi, urllib3, six, idna, cryptography, charset-normalizer, certifi, requests, pyOpenSSL, vapi-runtime, vapi-common-client, vmc-draas-client-bindings, vmc-client-bindings, vapi-client-bindings, pyVmomi, nsx-vmc-policy-python-sdk, nsx-vmc-aws-integration-python-sdk, nsx-python-sdk, nsx-policy-python-sdk, lxml, vSphere-Automation-SDK
Successfully installed certifi-2021.5.30 cffi-1.14.6 charset-normalizer-2.0.6 cryptography-3.4.8 idna-3.2 lxml-4.6.3 nsx-policy-python-sdk-3.1.2.1.1 nsx-python-sdk-3.1.2.1.1 nsx-vmc-aws-integration-python-sdk-3.1.2.1.1 nsx-vmc-policy-python-sdk-3.1.2.1.1 pyOpenSSL-20.0.1 pyVmomi-7.0.2 pycparser-2.20 requests-2.26.0 six-1.16.0 urllib3-1.26.6 vSphere-Automation-SDK-1.69.0 vapi-client-bindings-3.6.0 vapi-common-client-2.25.0 vapi-runtime-2.25.0 vmc-client-bindings-1.52.0 vmc-draas-client-bindings-1.18.0

... I don't see your code importing anything from suds(-jurko):

(pr287) root [ / ]# grep -R suds /tmp/pr287/
/tmp/pr287/lib64/python3.9/site-packages/vSphere_Automation_SDK-1.69.0.dist-info/direct_url.json:{"url": "https://github.com/mariolenz/vsphere-automation-sdk-python.git", "vcs_info": {"commit_id": "0457540d284c53e5f16a7a709d80145f21dc9166", "requested_revision": "remove_suds-jurko_dependency", "vcs": "git"}}
/tmp/pr287/lib/python3.9/site-packages/vSphere_Automation_SDK-1.69.0.dist-info/direct_url.json:{"url": "https://github.com/mariolenz/vsphere-automation-sdk-python.git", "vcs_info": {"commit_id": "0457540d284c53e5f16a7a709d80145f21dc9166", "requested_revision": "remove_suds-jurko_dependency", "vcs": "git"}}
(pr287) root [ / ]#

So I'm quite sure my PR won't break anything.

Did you find the time yet to have a look into this?

Akasurde commented 3 years ago

LGTM

Akasurde commented 3 years ago

@shwetapurohit Thanks for the review and merge. @mariolenz Thanks for the fix.

bradhvr commented 3 years ago

@shwetapurohit Will there be a new tag/release for this change?

mariolenz commented 3 years ago

Thanks for reviewing and merging @shwetapurohit!