Update Python 3 CI Configuration

NYPL-Simplified / server_core

Shared data model and utilities for Library Simplified server applications

7 stars 11 forks source link

Update Python 3 CI Configuration #1249

Closed jonathangreen closed 3 years ago

jonathangreen commented 3 years ago

I wanted to make a suggestion for how we setup CI for Python 3. I pulled this file out of the Python 3 branch I had going, and applied it to @EdwinGuzman Python 3 branch.

This PR:

Sets up CI testing to run against multiple Python versions and simplifies the CI config
Uses tox-docker to simplify the process of testing against containers
Adds some notes to the readme about different options you can pass to tox

@EdwinGuzman I made this as PR onto your Python 3 branch, since its really just a small add on to that branch. If you want me to target another branch, or wait until the Python 3 PR is merged, just let me know and I'll change the PR.

This PR is showing that branch failing against Python 3.9. I can remove 3.9 from the CI config if we don't want to test against 3.9 for now, but it might be good to support Python 3.9 so I've left it in there for now.

EdwinGuzman commented 3 years ago

This looks great and pointing to the py3-base branch is fine. I'll test it out soon. I don't have any concerns but was just confused the first time around because I have instances of postgres and elastic search running locally and they conflicted with the docker-based examples and ports (since they expected specific ports which are not the default).

EdwinGuzman commented 3 years ago

The setup works well for me for python 3.6 and 3.7 but not 3.8 or 3.9. I don't have python 3.8 or 3.9 installed on my machine and that's probably why. I get

ERROR:  py38-docker: InterpreterNotFound: python3.8
ERROR:  py39-docker: InterpreterNotFound: python3.9

though I wasn't expecting this.

jonathangreen commented 3 years ago

That makes sense, I see a similar error when testing with versions that aren't on my system. I can add a bit to the readme about that as well. Tox will look for python versions on your system to test with, but it won't actually go install a new version.

If your on OSX, I'm using pyenv to install all the different python versions to test against. Then it runs though them all. If you want to test with just the python versions you have installed you can do something like: tox -e "py{36,37}-docker"

So tox just tests those versions locally.

EdwinGuzman commented 3 years ago

Great, that works. Do you mind adding that info into the readme?

It seems there's a minor issue with python 3.9. Let's leave the config there and see if I can get that fixed.

jonathangreen commented 3 years ago

@EdwinGuzman updated the README with that extra info.

EdwinGuzman commented 3 years ago

Thanks!

tdilauro commented 3 years ago

It seems there's a minor issue with python 3.9. Let's leave the config there and see if I can get that fixed.

@EdwinGuzman One issue with 3.9 may be that base64.decodestring/encodestring were removed.

base64.encodestring() and base64.decodestring(), aliases deprecated since Python 3.1, have been removed: use base64.encodebytes() and base64.decodebytes() instead. (Contributed by Victor Stinner in bpo-39351.)

EdwinGuzman commented 3 years ago

Yes, I'm trying to update those functions to decodebytes and encodebytes but I'm running into some issues. I encountered this problem in circulation because I don't think decodestring or encodestring are used in server_core.

EdwinGuzman commented 3 years ago

This last commit makes all the test pass in 3.9 - https://github.com/NYPL-Simplified/server_core/commit/e00243a992bd60e6fe79c7d5f79466614799dc85

Looks like etree.getchildren() was deprecated and only gives warnings in 3.6-3.8. In 3.9 it throws an error.