Conversation
Geekdude
commented
Feb 5, 2026
- Add Dockerfile (.github/Dockerfile) to build image with Python 3.11, hatch, and dependencies
- Build Docker image in initial build job and push to ghcr.io with SHA and latest tags
- Add automatic cleanup to retain only 5 most recent images (cost management)
- Refactor test, coverage, and docs jobs to use container (eliminates venv recreation)
- Enable CI on all branches and PRs for full merge request support
- Ensure consistent environment across all dependent jobs
Format resultsMetrics Formatter output (last 200 lines) |
There was a problem hiding this comment.
Pull request overview
This PR implements a container-based CI workflow that builds a Docker image containing Python 3.11, hatch, and project dependencies, then uses this image across multiple CI jobs to eliminate redundant environment setup. The workflow includes automatic image cleanup to manage storage costs.
Changes:
- Added Dockerfile that builds an image with Python 3.11, hatch, and all project dependencies pre-installed
- Refactored CI workflow to build and push Docker images to GitHub Container Registry with SHA and latest tags
- Migrated docs, test, and coverage jobs to use the container image instead of recreating virtual environments
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| .github/Dockerfile | New Dockerfile that installs Python dependencies, hatch, and converts SSH URLs to HTTPS for package installation |
| .github/workflows/ci.yml | Refactored build job to create Docker image; updated docs, test, and coverage jobs to run in containers; added image cleanup step |
Comments suppressed due to low confidence (1)
.github/workflows/ci.yml:222
- When using GitHub Actions containers, the actions/checkout action may encounter permission issues with git operations due to ownership mismatches between the container user and the checked-out files. Consider adding 'options: --user root' to the container configuration or using 'git config --global --add safe.directory /github/workspace' in a setup step to avoid potential git permission errors.
name: coverage
path: |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Clean up old images | ||
| uses: actions/delete-package-versions@v4 | ||
| with: | ||
| package-name: 'ChatHPC-app' |
There was a problem hiding this comment.
The package name 'ChatHPC-app' appears to be hardcoded, but this may not match the actual GitHub repository name format. GitHub container registry packages typically use lowercase names. The repository appears to be 'ORNL/ChatHPC-app' based on the README, so the package name in GHCR would likely be 'chathpc-app' (lowercase). This mismatch could cause the cleanup action to fail or target the wrong package.
| package-name: 'ChatHPC-app' | |
| package-name: ${{ toLower(github.event.repository.name) }} |
| RUN pip install --no-cache-dir hatch hatch-mkdocs | ||
|
|
||
| # Convert SSH URLs to HTTPS in pyproject.toml | ||
| RUN ./scripts/pyproject_ssh_to_https.sh |
There was a problem hiding this comment.
The script execution assumes that the script file is executable, but the COPY command in Docker does not preserve file permissions. Consider either running the script with an explicit shell (e.g., 'RUN bash ./scripts/pyproject_ssh_to_https.sh') or adding a RUN command to make it executable before running it (e.g., 'RUN chmod +x ./scripts/pyproject_ssh_to_https.sh && ./scripts/pyproject_ssh_to_https.sh').
| RUN ./scripts/pyproject_ssh_to_https.sh | |
| RUN bash ./scripts/pyproject_ssh_to_https.sh |
| password: ${{ secrets.GITHUB_TOKEN }} | ||
| - name: Build and push Docker image | ||
| uses: docker/build-push-action@v5 | ||
| with: |
There was a problem hiding this comment.
Building the Docker image with the entire source code context (context: .) may include sensitive files or unnecessary build artifacts. Consider using a .dockerignore file to exclude unnecessary files and reduce the image size and build time. Common exclusions include .git/, .venv/, pycache/, *.pyc, .pytest_cache/, coverage_html_report/, and test-results.xml.
| @@ -15,43 +15,48 @@ jobs: | |||
| runs-on: ubuntu-latest | |||
There was a problem hiding this comment.
The build job pushes images to ghcr.io and uses actions/delete-package-versions to clean up old images, but there are no explicit permissions declared for the job. While GITHUB_TOKEN has default permissions, it's a best practice to explicitly declare required permissions using 'permissions:' block. Consider adding 'permissions: packages: write' to ensure the job has the necessary permissions to push images and delete package versions.
| runs-on: ubuntu-latest | |
| runs-on: ubuntu-latest | |
| permissions: | |
| packages: write |
| # Copy source code | ||
| COPY . . | ||
|
|
||
| # Install hatch | ||
| RUN pip install --no-cache-dir hatch hatch-mkdocs | ||
|
|
||
| # Convert SSH URLs to HTTPS in pyproject.toml | ||
| RUN ./scripts/pyproject_ssh_to_https.sh | ||
|
|
||
| # Install the package and all dependencies | ||
| RUN pip install --no-cache-dir . |
There was a problem hiding this comment.
The Dockerfile copies all source code before installing hatch and dependencies. This means any source code change will invalidate the Docker cache for all subsequent layers, including package installations. Consider reordering the Dockerfile to copy only pyproject.toml and necessary dependency files first, install dependencies, and then copy the rest of the source code. This would improve build times by leveraging Docker layer caching when only source code changes.
| hatch run docs:build | ||
| - name: Upload docs site |
There was a problem hiding this comment.
When using GitHub Actions containers, the actions/checkout action may encounter permission issues with git operations due to ownership mismatches between the container user and the checked-out files. Consider adding 'options: --user root' to the container configuration or using 'git config --global --add safe.directory /github/workspace' in a setup step to avoid potential git permission errors.
| hatch run test -- --junit-xml=test-results.xml | ||
| - name: Publish test results |
There was a problem hiding this comment.
When using GitHub Actions containers, the actions/checkout action may encounter permission issues with git operations due to ownership mismatches between the container user and the checked-out files. Consider adding 'options: --user root' to the container configuration or using 'git config --global --add safe.directory /github/workspace' in a setup step to avoid potential git permission errors.
| package-name: 'ChatHPC-app' | ||
| package-type: 'container' | ||
| min-versions-to-keep: 5 | ||
| delete-only-untagged-versions: false |
There was a problem hiding this comment.
The 'delete-only-untagged-versions' parameter is set to false, which means this cleanup will delete tagged versions (including the 'latest' tag) when more than 5 versions exist. This could break concurrent or subsequent jobs that depend on the 'latest' tag if they start after cleanup completes. Consider setting this to true to only delete untagged versions, or restructure the cleanup to run in a separate workflow after all jobs complete.
| delete-only-untagged-versions: false | |
| delete-only-untagged-versions: true |
| # Copy source code | ||
| COPY . . |
There was a problem hiding this comment.
The Docker image includes the entire source code at build time (line 12: COPY . .), but the container jobs also check out the code again (line 166: actions/checkout@v4). This creates a redundant copy of the source code and could lead to confusion about which version is being used. Consider either removing the COPY command from the Dockerfile and only checking out in the workflow, or removing the checkout steps from container jobs and relying on the image's built-in source.
| with: | ||
| context: . | ||
| file: .github/Dockerfile | ||
| push: true |
There was a problem hiding this comment.
The Docker image is tagged with both the commit SHA and 'latest', but dependent jobs reference the SHA tag specifically (e.g., line 164, 184, 222). This is good for reproducibility, but if the image cleanup (lines 37-43) runs before dependent jobs start and there are more than 5 versions, it could delete the SHA-tagged image that the jobs depend on, causing failures. The cleanup should either run after all dependent jobs complete or be moved to a separate scheduled workflow.
598fc64 to
8d0c4ed
Compare
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
29b1cb6 to
63b1ba3
Compare
williamfgc
left a comment
There was a problem hiding this comment.
LGTM, nice job! I'd just pick a more representative name (e.g. CI-GitHub-runners). We can generate a badge after this is merged. Thanks!
| @@ -1,5 +1,11 @@ | |||
| name: CI | |||
There was a problem hiding this comment.
I'd pick a more representative name in case we want to add CI on ExCL in the future.
There was a problem hiding this comment.
Thanks, I've renamed it.
- Add Dockerfile (.github/Dockerfile) to build image with Python 3.11, hatch, and dependencies - Build Docker image in initial build job and push to ghcr.io with SHA and latest tags - Convert repository name to lowercase for Docker tags (Docker requirement) - Add automatic cleanup to retain only 5 most recent images (cost management) - Refactor all jobs (format, docs, test, scripts, coverage) to use container - All jobs now depend on build and use consistent pre-built environment - Run SSH-to-HTTPS conversion after checkout in container jobs (fixes git clone issue) - Enable CI on all branches and PRs for full merge request support - Ensure consistent environment across all jobs
This commit modernizes the GitHub CI workflow to match improvements from the GitLab CI merge (feature-improve-python-template), bringing the two CI systems into sync. Major changes: Docker image: - Migrate from pip/hatch to uv for dependency management - Install uv via official installer script - Use `uv venv --seed --python 3.11` and `uv sync --all-groups` - Activate venv by default via PATH environment variable - Maintain aggressive cleanup of pycache/pyc files for space efficiency CI workflow: - Add test step in build job to verify image with `uv run python` - Update all job commands to use `uv run` prefix: * check_format: `uv run hatch fmt` * docs: `uv run hatch run docs:build` * test: `uv run hatch run test` * coverage: `uv run hatch run cov-html` New check_type job: - Add type checking with `uv run --all-groups ty check` - Provide comprehensive reporting (artifacts, summaries, PR comments) - Set as continue-on-error to allow workflow to proceed on failures - Capture metrics (error count, exit code) and full output - Post sticky PR comments with type check results Benefits: - Faster dependency resolution and installation with uv - Better caching and reproducibility - Consistency between GitLab and GitHub CI pipelines - Enhanced visibility into type checking issues
Type check resultsMetrics Type checker output (last 200 lines) |
Its not only for CI now.
…from cache when running.