Learnosity / learnosity-sdk-python

Learnosity SDK for Python
Apache License 2.0
7 stars 10 forks source link

[VENDOR] Update Dependencies LRN-32078 #53

Closed rhiannon-eldridge-lrn closed 2 years ago

rhiannon-eldridge-lrn commented 3 years ago

Checklist

david-scarratt-lrn commented 2 years ago

The Makefile changes may seem over-engineered. The point is to provide an environment that facilitates development and testing in whatever configuration a developer prefers: someone with Python experience and n Python versions installed locally should be able to work with a standard Python virtual environment, while someone doing FR with little Python experience and no local (standard) installation should be able to spin up a standard environment and run tests there. This complexity is pushed into the Makefile, so the developer shouldn't need to worry (too much) about it. The structure is explained in the notes at the end of the Makefile, but basically it should be usable the same as other SDKs.

shtrom commented 2 years ago

I think we should mandate one development configuration. Being too flexible is nice to the developer, but this means we might have to support a large combination of different issues that are linked to any custom build setup rather than the code.

Could we instead try to unify, across the SDKs, to something like a simple docker -v from a official upstream container for the language?

david-scarratt-lrn commented 2 years ago

That is how the Java and Ruby SDKs are set up (actually, Ruby is still in code review). It would have been the same here except that test-integration-dev just runs tox, and tox.ini contains all the supported Python versions. Running in a single-version container will result in errors from tox. I am less concerned about the support aspect than about having to keep tox.ini and the the multi-version container up to date every time the versions change.

Maybe it would be better to remove the envlist from tox.ini and run tox -e<version>, where the developer specifies the version (or a default is used). Then the build pulls the single-version container for the specified version and the container and tox match. It loses the nice multi-env aspect of tox, but it's not hard to run make test in a loop over versions instead. Then only the default version would need to be updated when versions change, like the other SDKs.

shtrom commented 2 years ago

The Makefiles for both the Java https://github.com/Learnosity/learnosity-sdk-java/blob/master/Makefile and Ruby https://github.com/Learnosity/learnosity-sdk-ruby/blob/ba08a9a95e05015794488438d13da3d3647d496f/Makefile SDKs are substantially shorter and sweeter. We should definitely try to aim to something similar here.

Ideally, we may even be able to dispense of bespoke Dockerfile / docker-compose

david-scarratt-lrn commented 2 years ago

We decided that this PR, which is supposed to be for a security patch, was being taken over by a separate issue. I have taken my changes out and put them in a separate PR, and reverted this one. Apologies for the mess.