Standardize development dependencies / refactor GHA workflow #153
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tillprochaska
force-pushed
the
fix/dev-dependencies
branch
3 times, most recently
from
9ae3dbf
to
ba5f314
Compare
tillprochaska
requested review from
catileptic and
stchris
and removed request for
catileptic
tillprochaska
force-pushed
the
fix/dev-dependencies
branch
9 times, most recently
from
27c61ed
to
ed8d2a5
Compare
tillprochaska
changed the title
catileptic
reviewed
Apr 24, 2024
|
|
||
| dev: | ||
| python3 -m pip install --upgrade pip setuptools | ||
| python3 -m pip install -q -r requirements.txt | ||
| python3 -m pip install -q -r requirements-dev.txt | ||
|
|
||
| test: | ||
| docker-compose run --rm shell pytest --cov=servicelayer | ||
| pytest --cov=servicelayer |
There was a problem hiding this comment.
test:
docker-compose run --rm shell pytest --cov=servicelayer
test-local:
pytest --cov=servicelayer
There seems to be a preference towards using `requirements-dev.txt` vs. a `dev` extra for development dependencies, but right now neither `pip install -r requirements-dev.txt` nor `pip install -e ".[dev]" is enough to install all development dependencies. I’ve added any dependencies listed in the `dev` extra to `requirements-dev.txt` and removed the `dev` extra so that we only have one list of dev dependencies to maintain. This also pins the `moto` package (used to mock S3 in tests) to version 4.x as version 5 release a few days ago contains breaking changes.
This seems to be left over from when we still used to run tests directly in GHA runners.
Tests in `test_extensions.py` rely on `servicelayer` being installed as they ultimately read information from the egg info that is created during installation. This is basically equivalent to the previous setup which executed `pip install -e .` outside of the container. As the entire directory is mounted into the container, the egg info subsequently was also available inside of the container. While that worked I found the fact that installing something outside of the container could make the tests fail or pass quite confusing. This should be a little more explicit.
Running some steps outside and some inside of the container leads to weird behavior, e.g. when file permissions/owners do not match across steps.
This uses a pre-built Python image based on Debian as this should be much faster than building a custom image on top of a full-feature Ubuntu.
tillprochaska
force-pushed
the
fix/dev-dependencies
branch
from
ed8d2a5
to
4b13998
Compare
tillprochaska
force-pushed
the
fix/dev-dependencies
branch
from
4b13998
to
41b2c0e
Compare
catileptic
approved these changes
Jun 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains a few changes loosely related to the management of development dependencies and the CI setup.
I initially started out trying to install development dependencies locally, but neither
pip install -e .[dev]norpip install -r requirements-dev.txtalone were enough as some dependencies are only specified in thedevextra and others only in the requirements file.As there seems to be a preference towards using
requirements-dev.txtvs. adevextra for development dependencies, I’ve added any dependencies listed in thedevextra torequirements-dev.txtand removed thedevextra so that we only have one list of dev dependencies to maintain.Along the way I noticed a few quirks with the setup of the GitHub Actions workflow which made it difficult to reproduce and debug issues so I’ve tried to clean up the setup a little bit by removing unnecessary steps (see commit messages for details). Some of these changes are probably a little opinionated, so feel free to object if you don't like them.
Pointing out two changes here explicitly as they might impact your preferred workflows:
I’ve changed the Docker base image to a Python slim image -- this helps bring down CI run times from ~3 mins to ~1 min, but it means that some CLI utils will not be installed by default. Not sure if this is an issue?
The
make testtarget doesn’t run pytest in a new container by default. Instead you’d have to rundocker compose run --rm shell make test(or something likedcr shell make testif you set up an alias). Or you just keep a container shell session open while developing and runmake testinside of that.