-
Notifications
You must be signed in to change notification settings - Fork 67
Adding Dockerfiles #354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Adding Dockerfiles #354
Conversation
Changed Requirements.txt minor version and added docker file for doc envinroment plus generation bash script for easy regeneration of the documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds a Docker-based workflow for building the Sphinx documentation, and updates a pinned Python dependency version to support that doc build environment.
Changes:
- Bump
PyYAMLpin from6.0to6.0.3inrequirements.txt. - Add a minimal doc-build Docker image (
docker/Dockerfile) and a helper script (docker.sh) to rebuild docs via Docker. - Add a build-hash file used by the helper script (
.docker.buildhash).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
requirements.txt |
Updates a pinned dependency version (PyYAML) for the docs toolchain. |
docker/Dockerfile |
Introduces a Python-based doc build image with git installed and requirements installed. |
docker.sh |
Adds a local helper to build (when inputs change) and run the doc generation in Docker. |
.docker.buildhash |
Stores the last build input hash for docker.sh’s incremental rebuild behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| VOLUME ["/app"] | ||
|
|
||
| COPY requirements.txt /app/requirements.txt | ||
| RUN pip install -r /app/requirements.txt |
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pip install without --no-cache-dir will leave pip caches in the image and bloat it unnecessarily. Consider using no-cache installs (and optionally disabling pip version checks) to keep the doc image smaller and more reproducible.
| RUN pip install -r /app/requirements.txt | |
| RUN pip install --no-cache-dir -r /app/requirements.txt |
| @@ -0,0 +1 @@ | |||
| 36b75867b3ce4c64223986f2c00d57faec8c5049359b5ad6e67ea0f8d5649dee | |||
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file looks like a local build artifact produced by docker.sh and will change whenever requirements.txt or the Dockerfile changes. Committing it will create churn and conflicts; it should typically be removed from the repo and ignored instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remove this. Should not be be in git
| IMAGE=age-doc | ||
| HASH_FILE=".docker.buildhash" | ||
|
|
||
| if command -v sha256sum >/dev/null 2>&1; then | ||
| HASH_CMD="sha256sum" | ||
| else | ||
| HASH_CMD="shasum -a 256" | ||
| fi | ||
|
|
||
| NEW_HASH=$( | ||
| $HASH_CMD docker/Dockerfile requirements.txt | $HASH_CMD | awk '{print $1}' | ||
| ) |
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script has no shebang and no strict error handling; running it as ./docker.sh may fail (depending on shell) and a failed docker build will still proceed to docker run. Add a #!/usr/bin/env bash shebang and enable strict mode (e.g., exit-on-error and unset-variable checks) so failures stop the script early.
| HASH_FILE=".docker.buildhash" | ||
|
|
||
| if command -v sha256sum >/dev/null 2>&1; then | ||
| HASH_CMD="sha256sum" | ||
| else | ||
| HASH_CMD="shasum -a 256" | ||
| fi | ||
|
|
||
| NEW_HASH=$( | ||
| $HASH_CMD docker/Dockerfile requirements.txt | $HASH_CMD | awk '{print $1}' | ||
| ) | ||
|
|
||
| if [ ! -f "$HASH_FILE" ] || [ "$NEW_HASH" != "$(cat "$HASH_FILE")" ]; then | ||
| docker build -f docker/Dockerfile -t "$IMAGE" . | ||
| echo "$NEW_HASH" > "$HASH_FILE" | ||
| fi |
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script writes to .docker.buildhash in the repo root. Since that file is committed in this PR, running the script will constantly dirty the working tree and can cause needless merge conflicts. Consider not committing the hash file (and adding it to .gitignore), or storing it under an ignored location (e.g., in a temp/cache dir).
| FROM python:3.11-slim | ||
|
|
||
| RUN apt-get update \ | ||
| && apt-get install -y --no-install-recommends git \ |
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs configuration enables sphinx.ext.imgmath (SVG output). That typically requires LaTeX + a converter like dvisvgm; the image currently only installs git, so doc builds may fail or omit math rendering. Install the required system packages in the Dockerfile (or switch to a math renderer that doesn’t require TeX tooling) so the container reliably builds the docs.
| && apt-get install -y --no-install-recommends git \ | |
| && apt-get install -y --no-install-recommends git texlive-latex-base dvisvgm \ |
|
@tcolo can you please review copilot's suggestions |
Changed Requirements.txt minor version and added docker file for doc envinroment plus generation bash script for easy regeneration of the documentation