Migrated from pip to uv #374

Merged

johnsturgeon merged 2 commits from migrate-to-uv-and-pyproject-toml-343 into main

2026-08-13 21:21:35 +02:00

johnsturgeon commented

2026-08-13 20:55:52 +02:00

Owner

No description provided.

johnsturgeon added 1 commit

2026-08-13 20:55:52 +02:00

Migrated from pip to uv

Claude Code Review / claude-review (pull_request) Successful in 1m50s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 17s

Details

18257f8021

forgejo-actions commented

2026-08-13 20:57:44 +02:00

I've reviewed the diff and the key files it touches. This is a clean, well-documented migration from pip/requirements.txt to uv/pyproject.toml, and the surrounding comments do a good job explaining the non-obvious choices (editable install + /srv, uv run avoided at runtime, dev group excluded from the image). The mechanics are sound; my findings are minor.

Review: Migrated from pip to uv (#374)

Overall this is a careful, well-reasoned migration and the resolution is preserved 1:1 from config/requirements.txt, with the version now sourced from package metadata. Nothing here is blocking; the notes below are about a couple of small correctness/robustness gaps and test coverage.

Worth fixing

scripts/test_and_lint.sh won't fail on a failing test or lint. The script uses set -uo pipefail but not -e, and the final command is flake8 ... --exit-zero. So if uv run pytest or uv run pylint fails, execution continues and the script's exit status is that of the --exit-zero flake8 call — i.e. 0. A developer running the local gate would see a green exit even with failing tests or pylint errors. This matches the pre-migration behavior, but the new set -uo pipefail header signals an intent to be strict that -e (or an explicit accumulator) would actually deliver. scripts/test_and_lint.sh:4
Stale Alembic fallback path in the entrypoint. docker/entrypoint.sh:24 still defaults to /app/alembic.ini (${ALEMBIC_CONFIG:-/app/alembic.ini}). The image now lives under /srv and the Dockerfile sets ALEMBIC_CONFIG=/srv/alembic.ini, so this is harmless in the built image — but the fallback is now a path that doesn't exist, so any invocation with the env var unset would fail confusingly. Worth updating to /srv/alembic.ini to match the new layout. docker/entrypoint.sh:24
APP_VERSION can now lag the running code in production. app_version() reads the version baked into the distribution metadata at image build time (app/config/config.py:16), while compose.prod.yml still bind-mounts ./app:/srv/app. That means after a git pull that changes app code without rebuilding the image, the reported version (Sentry release=, cache-busting query string) will reflect the old build, not the running tree — a regression from the old behavior where create_prod_env.sh wrote APP_VERSION from config/version.env at env-generation time. The PR notes #370 removes these mounts, which resolves it; until then it's a real drift worth calling out. compose.prod.yml:10, app/config/config.py:30

Nits

No test coverage for app_version(). It's the one piece of genuinely new logic (the raise-on-PackageNotFoundError path in particular), and CI's pytest step is still a placeholder echo "pytest is not enabled yet" (.forgejo/workflows/lint.yml). Not new to this PR, but the migration is a natural moment to add a first real test.
Pre-push hook checks that pyproject.toml was touched, not that the version changed. .githooks/pre-push:35 greps for the filename in the diff; a pyproject.toml edit that leaves version untouched still satisfies the gate. This is identical to the old config/version.env behavior, so no regression — just noting the guarantee is weaker than "the version was bumped."
Wheel asset packaging for the no-mount future. With packages = ["app"] and the editable install, templates/static under app/ are read straight from /srv/app today, so everything works. Once #370 drops the bind mounts, it'd be worth confirming app/templates/*.j2 and any static assets actually ship inside the wheel. Nothing to change now.

I've reviewed the diff and the key files it touches. This is a clean, well-documented migration from pip/requirements.txt to uv/pyproject.toml, and the surrounding comments do a good job explaining the non-obvious choices (editable install + `/srv`, `uv run` avoided at runtime, dev group excluded from the image). The mechanics are sound; my findings are minor. --- ## Review: Migrated from pip to uv (#374) Overall this is a careful, well-reasoned migration and the resolution is preserved 1:1 from `config/requirements.txt`, with the version now sourced from package metadata. Nothing here is blocking; the notes below are about a couple of small correctness/robustness gaps and test coverage. ### Worth fixing - **`scripts/test_and_lint.sh` won't fail on a failing test or lint.** The script uses `set -uo pipefail` but not `-e`, and the final command is `flake8 ... --exit-zero`. So if `uv run pytest` or `uv run pylint` fails, execution continues and the script's exit status is that of the `--exit-zero` flake8 call — i.e. `0`. A developer running the local gate would see a green exit even with failing tests or pylint errors. This matches the pre-migration behavior, but the new `set -uo pipefail` header signals an intent to be strict that `-e` (or an explicit accumulator) would actually deliver. `scripts/test_and_lint.sh:4` - **Stale Alembic fallback path in the entrypoint.** `docker/entrypoint.sh:24` still defaults to `/app/alembic.ini` (`${ALEMBIC_CONFIG:-/app/alembic.ini}`). The image now lives under `/srv` and the Dockerfile sets `ALEMBIC_CONFIG=/srv/alembic.ini`, so this is harmless in the built image — but the fallback is now a path that doesn't exist, so any invocation with the env var unset would fail confusingly. Worth updating to `/srv/alembic.ini` to match the new layout. `docker/entrypoint.sh:24` - **`APP_VERSION` can now lag the running code in production.** `app_version()` reads the version baked into the distribution metadata at image build time (`app/config/config.py:16`), while `compose.prod.yml` still bind-mounts `./app:/srv/app`. That means after a `git pull` that changes app code without rebuilding the image, the reported version (Sentry `release=`, cache-busting query string) will reflect the old build, not the running tree — a regression from the old behavior where `create_prod_env.sh` wrote `APP_VERSION` from `config/version.env` at env-generation time. The PR notes #370 removes these mounts, which resolves it; until then it's a real drift worth calling out. `compose.prod.yml:10`, `app/config/config.py:30` ### Nits - **No test coverage for `app_version()`.** It's the one piece of genuinely new logic (the raise-on-`PackageNotFoundError` path in particular), and CI's pytest step is still a placeholder `echo "pytest is not enabled yet"` (`.forgejo/workflows/lint.yml`). Not new to this PR, but the migration is a natural moment to add a first real test. - **Pre-push hook checks that `pyproject.toml` was *touched*, not that the version changed.** `.githooks/pre-push:35` greps for the filename in the diff; a `pyproject.toml` edit that leaves `version` untouched still satisfies the gate. This is identical to the old `config/version.env` behavior, so no regression — just noting the guarantee is weaker than "the version was bumped." - **Wheel asset packaging for the no-mount future.** With `packages = ["app"]` and the editable install, templates/static under `app/` are read straight from `/srv/app` today, so everything works. Once #370 drops the bind mounts, it'd be worth confirming `app/templates/*.j2` and any static assets actually ship inside the wheel. Nothing to change now.

johnsturgeon added 1 commit

2026-08-13 21:16:38 +02:00

All scripts, and now Docker use uv directly and pyproject is the source of truth for the version

Claude Code Review / claude-review (pull_request) Successful in 1m47s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 24s

Details

4b7f9aa09b

johnsturgeon merged commit 3b66905065 into main

2026-08-13 21:21:35 +02:00

johnsturgeon deleted branch migrate-to-uv-and-pyproject-toml-343

2026-08-13 21:21:35 +02:00