Copilot commented on code in PR #354:
URL: https://github.com/apache/age-website/pull/354#discussion_r2779445243
##########
docker.sh:
##########
@@ -0,0 +1,19 @@
+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}'
+)
Review Comment:
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.
##########
docker/Dockerfile:
##########
@@ -0,0 +1,11 @@
+FROM python:3.11-slim
+
+RUN apt-get update \
+ && apt-get install -y --no-install-recommends git \
+ && rm -rf /var/lib/apt/lists/*
+
+WORKDIR /app
+VOLUME ["/app"]
+
+COPY requirements.txt /app/requirements.txt
+RUN pip install -r /app/requirements.txt
Review Comment:
`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.
```suggestion
RUN pip install --no-cache-dir -r /app/requirements.txt
```
##########
docker/Dockerfile:
##########
@@ -0,0 +1,11 @@
+FROM python:3.11-slim
+
+RUN apt-get update \
+ && apt-get install -y --no-install-recommends git \
Review Comment:
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.
```suggestion
&& apt-get install -y --no-install-recommends git texlive-latex-base
dvisvgm \
```
##########
.docker.buildhash:
##########
@@ -0,0 +1 @@
+36b75867b3ce4c64223986f2c00d57faec8c5049359b5ad6e67ea0f8d5649dee
Review Comment:
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.
##########
docker.sh:
##########
@@ -0,0 +1,19 @@
+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}'
+)
+
+if [ ! -f "$HASH_FILE" ] || [ "$NEW_HASH" != "$(cat "$HASH_FILE")" ]; then
+ docker build -f docker/Dockerfile -t "$IMAGE" .
+ echo "$NEW_HASH" > "$HASH_FILE"
+fi
Review Comment:
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
