Python 3/2 backwards compatibility

dhaynespls commented 7 years ago

Description

Given the fact that Python 2 will be EOL as of 2020, it is a good idea to move development of new libraries to Python 3. Ensuring backwards compatibility with Python 2 is essential however since there are a number of developers who are in a situation where they do not have control over the environment and also to ensure that the Python 3 port won't break existing installs. This PR migrates the library to a state where it can be run 100% on Python 3 & 2.

Motivation and Context

See above.

How Has This Been Tested?

By running unit testing in both environments:

Python 2

[~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> python2 --version
Python 2.7.13
[~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> virtualenv --version
15.1.0
[~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> virtualenv -p python2 ../virtualenv/nx-2
Running virtualenv with interpreter /usr/bin/python2
New python executable in /home/dhaynes/development/virtualenv/nx-2/bin/python2
Also creating executable in /home/dhaynes/development/virtualenv/nx-2/bin/python
Installing setuptools, pip, wheel...done.
[~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> source ../virtualenv/nx-2/bin/activate.fish
(nx-2) [~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> pip install -r requirements.txt
Collecting lxml==3.6.0 (from -r requirements.txt (line 1))
Collecting future==0.16.0 (from -r requirements.txt (line 2))
Installing collected packages: lxml, future
Successfully installed future-0.16.0 lxml-3.6.0
(nx-2) [~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> pip install -r requirements_test.txt
Requirement already satisfied: lxml==3.6.0 in /home/dhaynes/development/virtualenv/nx-2/lib/python2.7/site-packages (from -r requirements_test.txt (line 1))
Collecting py==1.4.31 (from -r requirements_test.txt (line 2))
  Using cached py-1.4.31-py2.py3-none-any.whl
Collecting pytest==2.9.2 (from -r requirements_test.txt (line 3))
  Using cached pytest-2.9.2-py2.py3-none-any.whl
Requirement already satisfied: future==0.16.0 in /home/dhaynes/development/virtualenv/nx-2/lib/python2.7/site-packages (from -r requirements_test.txt (line 4))
Installing collected packages: py, pytest
Successfully installed py-1.4.31 pytest-2.9.2
(nx-2) [~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> py.test -v tests
============================================================================================================= test session starts =============================================================================================================
platform linux2 -- Python 2.7.13, pytest-2.9.2, py-1.4.31, pluggy-0.3.1 -- /home/dhaynes/development/virtualenv/nx-2/bin/python2
cachedir: .cache
rootdir: /home/dhaynes/development/nexpose-client-python, inifile:
collected 27 items

tests/test_LoadFixture.py::LoadFixtureTestCase::testThatLoadingInvalidFixtureTypeResultsInAnException PASSED
tests/test_LoadFixture.py::LoadFixtureTestCase::testThatLoadingNonExistingFixtureResultsInAnException PASSED
tests/test_LoadFixture.py::LoadFixtureTestCase::testThatOurFixturesWillLoadCorrectly PASSED
tests/test_NexposeNode.py::NexposeNodeTestCase::testCreateFromJSON PASSED
tests/test_NexposeReportConfigurationSummary.py::NexposeReportConfigurationSummaryTestCase::testCreateFromXML PASSED
tests/test_NexposeReportConfigurationSummary.py::NexposeReportSummaryTestCase::testCreateFromXML PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testConstructionOfLoginRequest PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testConstructionOfURI_APIv1d1 PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testCorrectLogin PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testDefaultConstructionOfURI_APIv1d1 PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testIncorrectLogin PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testLoginWithInvalidHtmlReply PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testRequestSiteDeviceListing PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testRequestSiteListing PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testRequestSiteScanHistory PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testRequestSystemInformation PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testShouldNotLoginIfSessionIsOpen PASSED
tests/test_NexposeTag.py::NexposeTagAttributeTestCase::testCreate PASSED
tests/test_NexposeTag.py::NexposeTagTestCase::testCreate PASSED
tests/test_NexposeTag.py::NexposeTagTestCase::testCreateCustom PASSED
tests/test_NexposeTag.py::NexposeTagTestCase::testCreateFromJSON PASSED
tests/test_NexposeTag.py::NexposeTagTestCase::testCustomTagWithAttributes PASSED
tests/test_NexposeUserAuthenticatorSummary.py::NexposeUserAuthenticatorSummaryTestCase::testCreateFromXML PASSED
tests/test_NexposeVulnerability.py::NexposeVulnerabilitySummaryTestCase::testCreatingFromXML PASSED
tests/test_NexposeVulnerability.py::NexposeVulnerabilitySummaryTestCase::testIfAnEmptyXmlElementResultsInDefaultValues PASSED
tests/test_NexposeVulnerability.py::NexposeVulnerabilityDetailTestCase::testCreatingFromXML PASSED
tests/test_NexposeVulnerability.py::NexposeVulnerabilityDetailTestCase::testIfAnEmptyXmlElementResultsInDefaultValues PASSED

========================================================================================================== 27 passed in 0.17 seconds ==========================================================================================================
(nx-2) [~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> deactivate

Python 3

[~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─(1)-> python3 --version
Python 3.6.2
[~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> virtualenv -p python3 ../virtualenv/nx-3
Running virtualenv with interpreter /usr/bin/python3
Using base prefix '/usr'
New python executable in /home/dhaynes/development/virtualenv/nx-3/bin/python3
Not overwriting existing python script /home/dhaynes/development/virtualenv/nx-3/bin/python (you must use /home/dhaynes/development/virtualenv/nx-3/bin/python3)
Installing setuptools, pip, wheel...done.
[~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> source ../virtualenv/nx-3/bin/activate.fish
(nx-3) [~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> pip install -r requirements.txt
Collecting lxml==3.6.0 (from -r requirements.txt (line 1))
Collecting future==0.16.0 (from -r requirements.txt (line 2))
Installing collected packages: lxml, future
Successfully installed future-0.16.0 lxml-3.6.0
(nx-3) [~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> pip install -r requirements_test.txt
Requirement already satisfied: lxml==3.6.0 in /home/dhaynes/development/virtualenv/nx-3/lib/python3.6/site-packages (from -r requirements_test.txt (line 1))
Collecting py==1.4.31 (from -r requirements_test.txt (line 2))
  Using cached py-1.4.31-py2.py3-none-any.whl
Collecting pytest==2.9.2 (from -r requirements_test.txt (line 3))
  Using cached pytest-2.9.2-py2.py3-none-any.whl
Requirement already satisfied: future==0.16.0 in /home/dhaynes/development/virtualenv/nx-3/lib/python3.6/site-packages (from -r requirements_test.txt (line 4))
Installing collected packages: py, pytest
Successfully installed py-1.4.31 pytest-2.9.2
(nx-3) [~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> py.test -v tests
============================================================================================================= test session starts =============================================================================================================
platform linux -- Python 3.6.2, pytest-2.9.2, py-1.4.31, pluggy-0.3.1 -- /home/dhaynes/development/virtualenv/nx-3/bin/python3
cachedir: .cache
rootdir: /home/dhaynes/development/nexpose-client-python, inifile:
collected 27 items

tests/test_LoadFixture.py::LoadFixtureTestCase::testThatLoadingInvalidFixtureTypeResultsInAnException PASSED
tests/test_LoadFixture.py::LoadFixtureTestCase::testThatLoadingNonExistingFixtureResultsInAnException PASSED
tests/test_LoadFixture.py::LoadFixtureTestCase::testThatOurFixturesWillLoadCorrectly PASSED
tests/test_NexposeNode.py::NexposeNodeTestCase::testCreateFromJSON PASSED
tests/test_NexposeReportConfigurationSummary.py::NexposeReportConfigurationSummaryTestCase::testCreateFromXML PASSED
tests/test_NexposeReportConfigurationSummary.py::NexposeReportSummaryTestCase::testCreateFromXML PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testConstructionOfLoginRequest PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testConstructionOfURI_APIv1d1 PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testCorrectLogin PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testDefaultConstructionOfURI_APIv1d1 PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testIncorrectLogin PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testLoginWithInvalidHtmlReply PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testRequestSiteDeviceListing PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testRequestSiteListing PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testRequestSiteScanHistory PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testRequestSystemInformation PASSED
tests/test_NexposeSession.py::NexposeSessionTestCase::testShouldNotLoginIfSessionIsOpen PASSED
tests/test_NexposeTag.py::NexposeTagAttributeTestCase::testCreate PASSED
tests/test_NexposeTag.py::NexposeTagTestCase::testCreate PASSED
tests/test_NexposeTag.py::NexposeTagTestCase::testCreateCustom PASSED
tests/test_NexposeTag.py::NexposeTagTestCase::testCreateFromJSON PASSED
tests/test_NexposeTag.py::NexposeTagTestCase::testCustomTagWithAttributes PASSED
tests/test_NexposeUserAuthenticatorSummary.py::NexposeUserAuthenticatorSummaryTestCase::testCreateFromXML PASSED
tests/test_NexposeVulnerability.py::NexposeVulnerabilitySummaryTestCase::testCreatingFromXML PASSED
tests/test_NexposeVulnerability.py::NexposeVulnerabilitySummaryTestCase::testIfAnEmptyXmlElementResultsInDefaultValues PASSED
tests/test_NexposeVulnerability.py::NexposeVulnerabilityDetailTestCase::testCreatingFromXML PASSED
tests/test_NexposeVulnerability.py::NexposeVulnerabilityDetailTestCase::testIfAnEmptyXmlElementResultsInDefaultValues PASSED

========================================================================================================== 27 passed in 0.13 seconds ==========================================================================================================
(nx-3) [~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> deactivate

Screenshots (if appropriate):

See above.

Types of changes

New feature (non-breaking change which adds functionality): py3 support

Checklist:

[x] I have updated the documentation accordingly (if changes are required).
[] I have added tests to cover my changes. ^ not necessary as testing py3 involves just running tests in a python 3 enviornment
[x] All new and existing tests passed.

Closes #4

gschneider-r7 commented 7 years ago

I want to set aside some time to do a live usage test before merging, but this looks good to me otherwise. Thank you for doing this!

Did you find any notable differences or "gotchas" for environment setup in py3 compared to py2? In other words would a new user on py3 need additional/different steps than a py2 user when creating a virtual environment, etc? (besides specifically calling python3 vs python2)

dhaynespls commented 7 years ago

There shouldn't be anything additional that someone would need to do depending on what version of Python they use.

A situation that comes to mind is if in the future someone decides to contribute code that utilizes a Python 3 specific feature such as ipaddress or asyncio . In that case, in order to ensure Python 2 users aren't affected, they would need to include a back-ported package as a requirement. Luckily the repo in it's current state uses almost exclusively the Python standard library which is fairly interoperable on py2/3.

dhaynespls commented 7 years ago

I would suggest adding the travis-ci bot to PRs to do this checking as well as noting in documentation that contributions must be backwards compatible.

gschneider-r7 commented 7 years ago

I've made some changes to get things working on travis-ci, please rebase (or re-merge upstream/master) when you get a chance. Thanks!

gschneider-r7 commented 7 years ago

Nevermind, I was able to resolve the conflicts through github.

dhaynespls commented 7 years ago

Huh, a pass in 3.6 but fail in 3.5. I'll look a it.

 b'<LoginRequest password="nxpassword" user-id="nxadmin"/>' != b'<LoginRequest user-id="nxadmin" password="nxpassword"/>'

dhaynespls commented 7 years ago

You can replicate the issue with:

[~/d/nexpose-client-python]─[⎇ 4-py3-backwards-compat]─> python
Python 2.7.13 (default, Jun 26 2017, 10:20:05)
[GCC 7.1.1 20170622 (Red Hat 7.1.1-3)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> d = {'x': 1, 'y': 2}
>>> d.items()
[('y', 2), ('x', 1)]

gschneider-r7 commented 7 years ago

The test assertion may be too strict as the Nexpose API doesn't actually care about the order of the attributes, but that is interesting to see it behave differently on 3.5 specifically!

I think I'm okay with ignoring that failure for now and dealing with it another time.

As for testing, today I started using pyenv and pyenv-virtualenv to install and manage multiple python versions. Give it a try if you don't like waiting on travis-ci runs.

dhaynespls commented 7 years ago

Oh cool I wasn't aware of those tools, I'll have to check them out.

If it's no big deal then I won't worry about it, I was just trying to be thorough and not merge in any huge bugs.

gschneider-r7 commented 7 years ago

Did some local testing; Even with OrderedDict it's doing odd stuff and only on 3.5 (although I didn't check 3.3 or 3.4). It doesn't happen on every run either.

I think this can be "fixed" from a testing standpoint by setting the PYTHONHASHSEED environment variable to a static value in travis-ci. Locally it seems to work out that way at least.

You can probably just remove the OrderedDict change and then change .travis.yml like so:

script:
  - if [[ $TRAVIS_PYTHON_VERSION == 3.5 ]]; then export PYTHONHASHSEED=3140657756; fi
  - py.test tests

gschneider-r7 commented 7 years ago

Re-ran it a few times on travis and it passed on the last try 🙄

dhaynespls commented 7 years ago

passed on the last try

Oh wow that's interesting..

I think the ideal solution should be to:

1) Revert the OrderedDict commit 2) Refactor that test to check for either:

<LoginRequest password="nxpassword" user-id="nxadmin"/>
<LoginRequest user-id="nxadmin" password="nxpassword"/>

Given that the Nexpose API does not care about ordering, just that the attributes are present.

Worst case then fall back to:

script:
  - if [[ $TRAVIS_PYTHON_VERSION == 3.5 ]]; then export PYTHONHASHSEED=3140657756; fi
  - py.test tests

dhaynespls commented 7 years ago

Should be good now.

pyenv was a lifesaver there thanks for the share.

rapid7 / nexpose-client-python