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.

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

Add support for Python 2 and 3 #28

Merged
merged 14 commits into from
Oct 9, 2015
Merged

Add support for Python 2 and 3 #28

merged 14 commits into from
Oct 9, 2015

Conversation

saj
Copy link
Contributor

@saj saj commented Oct 9, 2015

Fixes #22.

Please note that this pull request will drop support for Python 2.5.

Tested with:

  • CPython 2.6.9
  • CPython 2.7.10
  • CPython 3.3.6
  • CPython 3.4.3

I was forced to add a handful of unit tests to ensure basic functionality across the different runtimes. These tests offer minimal coverage. Tests for paginated responses and writes are missing. The tests that do exist may differ from your house style.

@saj saj mentioned this pull request Oct 9, 2015
Drops support for Python 2.5.
@wbyoung
Copy link
Contributor

wbyoung commented Oct 9, 2015

👍

@gmjosack
Copy link
Contributor

gmjosack commented Oct 9, 2015

Thanks a lot for taking this on. Especially adding some basic unit tests.

Can you update the .travis.yml to run the new unit tests as well?

Saj Goonatilleke added 2 commits October 10, 2015 03:05
Tox is configured to test against multiple Python runtimes.  Travis
invokes Tox.  Travis was also configured to test against multiple Python
runtimes.  Something smelt wrong.

While this worked, I think your CI was doing unnecessary work:

  - Travis spawns 4x jobs on each build.  Travis provisions each job
    with an initial virtualenv.  In this initial Travis virtualenv,
    'python' resolves to whatever was configured in .travis.yml.  This
    python interpreter is used to install tox and start it.

  - Tox starts up.  Tox spawns 4x new virtualenvs.  In these Tox
    virtualenvs, 'python' resolves to whatever was configured in
    tox.ini.  These are the python interpreters that ultimately run the
    linter and unit tests.

The effect was multiplicative:  4 x 4 + 4 = 20 virtualenvs were being
spawned in CI.

I think this worked because Travis pre-provision a variety of Python
runtimes on all their job instances:  not just whichever runtime was
selected for the job from the 'python' sequence in .travis.yml.  All
runtimes are accessible at pythonX.Y, which is why all Tox environments
succeeded across all Travis jobs.

Following this commit, Travis will still spawn 4x parallel jobs on each
build, but Tox should limit itself to just one Python runtime target for
each Travis job.  This should make for fewer virtualenvs (4 + 4 = 8) and
faster CI builds.

This arrangement should also work in local testing (Tox without Travis).
@saj
Copy link
Contributor Author

saj commented Oct 9, 2015

Can you update the .travis.yml to run the new unit tests as well?

Done.

Even with the new unit tests in c938946, the changes to the Travis configuration in e6b9d20 have halved CI build times:

https://travis-ci.org/dropbox/pygerduty/builds/84478461
https://travis-ci.org/dropbox/pygerduty/builds/84546353

You might want to check your other projects for the same problem. More comprehensive (time consuming) test suites will amplify the negative effect.

Cheers.

@gmjosack
Copy link
Contributor

gmjosack commented Oct 9, 2015

Yeah, I'd noticed that problem as I asked you to add the tests. I don't normally use tox (that was another pull request) so thanks for fixing that up as well!

gmjosack added a commit that referenced this pull request Oct 9, 2015
Add support for Python 2 and 3
@gmjosack gmjosack merged commit dff6bf8 into dropbox:master Oct 9, 2015
@gmjosack
Copy link
Contributor

gmjosack commented Oct 9, 2015

This has been pushed up to PyPI as pygerduty-0.29.0

Thanks again for the work here!

@wbyoung
Copy link
Contributor

wbyoung commented Oct 9, 2015

Thanks @saj and @chris-martin & for the quick release @gmjosack.

@chris-martin
Copy link
Contributor

Thanks everybody :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants