Migrate to uv and gha#223
Conversation
- Updated Poetry from version 1.8.4 to 2.2.1 - Updated isort from version 5.13.2 to 7.0.0 - Updated black from version 24.10.0 to 25.9.0 - Removed poetry.lock file - Refactored pyproject.toml to use the new project structure and updated dependencies - Updated CircleCI configuration to use newer versions of docker, github-cli, and codecov
WalkthroughAdds a Docker Buildx bake config and a multi-arch build target; rewrites Dockerfile to use uv-based dependency sync and a pre-built virtualenv with non-root execution and Python 3.12, changes runtime port to 8080; appends Node.js ignore entries to Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI/Developer
participant Bake as docker buildx bake
participant Target as build target
participant Docker as Dockerfile
participant Builder as Builder stage (uv sync)
participant App as Final app image
CI->>Bake: trigger bake using `docker-bake.hcl` (group: default -> build)
Bake->>Target: run `build` (inherits docker-metadata-action)
Target->>Docker: provide context (`./`) and Dockerfile
Docker->>Builder: run uv sync to prepare dependencies and build prebuilt /app
Builder->>App: COPY --from=builder /app into final image (chown to non-root)
App-->>CI: produce multi-platform images (linux/amd64, linux/arm64/v8)
note right of App #E8F6FF: Final image runs non-root and exposes port 8080
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 11 11
Lines 213 212 -1
=========================================
- Hits 213 212 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pyproject.tomlis excluded by!**/*.tomluv.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (1)
Dockerfile(1 hunks)
🔇 Additional comments (2)
Dockerfile (2)
17-26: Clarify the purpose of two sequentialuv synccommands.Two
uv synccalls appear at lines 17–20 and 25–26 in different contexts:
- First:
uv sync --locked --no-dev --no-install-project(dependencies only, cached)- Second:
uv sync --locked --no-dev --no-editable(includes app, non-cached)If the second sync includes the full app installation, the first may be redundant. Confirm whether this two-stage approach is necessary (e.g., for layer caching optimization) or if it can be simplified.
1-1: Document base image version downgrade.The base image was downgraded from
python:3.13topython:3.12.12with SHA256 pinning. While reproducibility is improved, the reason for the downgrade should be documented (e.g., compatibility, security, stability). Ensure this is intentional and note it in commit messages or comments if not already done.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Dockerfile (1)
47-47: Non-root user cannot bind to port 80 — runtime failure expected.The CMD at line 47 binds uvicorn to port 80 while the container runs as
nonroot:nonroot(line 43). Binding to ports below 1024 requires root privileges, so this will fail at runtime with "Permission denied."Either switch to a port ≥1024 (e.g., 8080) or run as root:
Option 1 (Recommended): Use unprivileged port
-CMD ["uvicorn", "bss_web_file_server.main:app", "--host", "0.0.0.0", "--port", "80"] +CMD ["uvicorn", "bss_web_file_server.main:app", "--host", "0.0.0.0", "--port", "8080"]Then ensure external routing (load balancer, reverse proxy, or Kubernetes ingress) maps host port 80 → container port 8080.
Option 2 (If port 80 required inside container): Run as root
-USER nonroot:nonrootThis degrades security posture; Option 1 is preferred.
Option 3 (Advanced): Grant CAP_NET_BIND_SERVICE
Use Docker/Kubernetes capabilities to allow nonroot to bind to privileged ports (requires orchestration platform support).
🧹 Nitpick comments (1)
Dockerfile (1)
1-1: Pin builder base image by SHA256 hash.For supply-chain security and reproducibility, pin the Python base image to a specific digest. The uv image at line 3 is correctly pinned; apply the same pattern here.
-FROM python:3.12.12-alpine AS builder +FROM python:3.12.12-alpine@sha256:d82291d418d5c47f267708393e40599ae836f2260b0519dd38670e9d281657f5 AS builder
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/integration.ymlis excluded by!**/*.yml
📒 Files selected for processing (2)
Dockerfile(1 hunks)docker-bake.hcl(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Scorecard
Dockerfile
[warning] 1-1: Pinned-Dependencies
score is 8: containerImage not pinned by hash
Remediation tip: pin your Docker image by updating python:3.12.12-alpine to python:3.12.12-alpine@sha256:d82291d418d5c47f267708393e40599ae836f2260b0519dd38670e9d281657f5
Click Remediation section below for further remediation help
[warning] 32-32: Pinned-Dependencies
score is 8: containerImage not pinned by hash
Remediation tip: pin your Docker image by updating python:3.12-alpine to python:3.12-alpine@sha256:d82291d418d5c47f267708393e40599ae836f2260b0519dd38670e9d281657f5
Click Remediation section below for further remediation help
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and publish
🔇 Additional comments (3)
docker-bake.hcl (1)
1-18: Docker Bake configuration looks good.This configuration correctly sets up multi-arch builds for both amd64 and arm64. The inheritance from
docker-metadata-actionenables seamless integration with GitHub Actions for automatic tagging and metadata injection.Dockerfile (2)
35-44: Non-root setup is now correct.The PATH configuration (line 35) now correctly points to
/app/binwhere uv-created venv executables reside. User creation, WORKDIR alignment, and--chownflag are all properly set for non-root execution.
32-32: I found a discrepancy: line 1 is not actually pinned by SHA256 (it usespython:3.12.12-alpine), only line 3 uses SHA256 pinning. Let me get the current digest for the python:3.12-alpine image.Pin the app base image by SHA256 digest.
Line 1 (
python:3.12.12-alpine) is not actually pinned by SHA256, so the reference to consistency with line 1 in the original comment is inaccurate. However, line 3 demonstrates the correct pattern with digest pinning. For supply-chain security and reproducibility, pin line 32 using the manifest-list digest (not platform-specific):-FROM python:3.12-alpine AS app +FROM python:3.12-alpine@sha256:<MANIFEST_DIGEST> AS appObtain the manifest-list digest for
python:3.12-alpineusing:docker manifest inspect python:3.12-alpine | grep '"digest"' | head -1Or use tools like
crane manifest python:3.12-alpineorskopeo inspect --raw docker://python:3.12-alpine.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
.github/workflows/python.ymlis excluded by!**/*.yml.github/workflows/release.ymlis excluded by!**/*.yml.pre-commit-config.yamlis excluded by!**/*.yamldocker-compose.ymlis excluded by!**/*.ymlpyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (3)
.dockerignore(1 hunks)Dockerfile(1 hunks)tests-int/test_health.py(3 hunks)
🔇 Additional comments (8)
.dockerignore (1)
164-166: LGTM!The Node.js template addition follows standard conventions and properly ignores
node_modules/from the Docker build context.tests-int/test_health.py (1)
25-25: Port update correctly aligned with Dockerfile changes.The port references have been properly updated from 80 to 8080, matching the new EXPOSE directive and non-root user requirements in the Dockerfile.
Also applies to: 34-34
Dockerfile (6)
1-1: Base image properly pinned by digest.The Python base image now uses SHA256 digest pinning, addressing the security scanner warnings about unpinned container images.
5-10: UV integration configured correctly.The UV tool integration uses appropriate environment variables for a production Docker build: bytecode pre-compilation for faster startup, copy link mode for isolation, and disabled runtime downloads.
12-22: Dependency synchronization properly optimized.The two-phase dependency installation (dependencies first, then project) with cache mounts and bind mounts follows UV best practices for Docker builds, ensuring fast rebuilds when only source code changes.
24-31: Package installation approach is standard but verify project configuration.The build installs the application as a non-editable package into the venv, allowing the final image to exclude source files. Ensure your
pyproject.tomlcorrectly defines the package structure sobss_web_file_server.main:app(referenced in line 49) is importable from the installed package.
34-46: Non-root user implementation is secure and correct.The app stage properly creates a non-root user, transfers venv ownership, and configures PATH to include the venv executables. This follows Docker security best practices.
48-48: Directory creation is handled by the application at startup—verify file system permissions.The application's startup lifecycle (in
src/bss_web_file_server/main.py) callsvideo_service.create_base_path()andmember_service.create_base_path(), which automatically create/home/nonroot/assetsand subdirectories withmkdir(parents=True, exist_ok=True)if they don't exist.However, ensure that:
- The parent directory
/home/nonrootis accessible and writable by the nonroot user- The base image or earlier Dockerfile steps create
/home/nonrootwith proper permissions if needed
| build=True, | ||
| wait=True, | ||
| ) | ||
| compose.waiting_for({"app": LogMessageWaitStrategy("8080")}) |
There was a problem hiding this comment.
Use the actual startup message in LogMessageWaitStrategy.
The wait strategy currently waits for the string "8080" to appear in logs, which is fragile and may match unintended log entries (e.g., port configuration messages). Since line 26 confirms the actual startup message is "Application startup complete.", use that instead for reliable container readiness detection.
Apply this diff:
- compose.waiting_for({"app": LogMessageWaitStrategy("8080")})
+ compose.waiting_for({"app": LogMessageWaitStrategy("Application startup complete.")})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| compose.waiting_for({"app": LogMessageWaitStrategy("8080")}) | |
| compose.waiting_for({"app": LogMessageWaitStrategy("Application startup complete.")}) |
🤖 Prompt for AI Agents
In tests-int/test_health.py around line 17, the LogMessageWaitStrategy is
currently waiting for the string "8080" which is brittle; change it to wait for
the actual startup log "Application startup complete." so the container
readiness is detected reliably. Replace the wait strategy argument from "8080"
to "Application startup complete." (matching the exact message used on line 26)
so the test waits for the definitive startup log instead of a port substring.
BREAKING CHANGE: Move to uv from poetry and update all dependencies. Removed CircleCI badge from README. See #223
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
BREAKING CHANGES