Fix command substitution syntax in workflow#8891
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpdated CI Docker health check to capture Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pythonpackage.yml:
- Line 181: The pipeline `$(curl http://localhost:8081/home/ -s | grep -q
"site-notification-modal")` is wrapped in a command substitution, so its empty
stdout is executed instead of using grep's exit status; remove the `$()` so the
pipeline `curl ... | grep -q "site-notification-modal"` runs directly and its
exit code controls the `&& echo "Success!" || (echo "Failed" && exit 1)` branch,
ensuring the failure path triggers correctly when the pattern is not found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58ff48d1-a2ee-4803-85fd-b238e1c6cf9e
📒 Files selected for processing (1)
.github/workflows/pythonpackage.yml
Updated Python version from 3.10 to 3.11 in Dockerfile and added startup logging for debugging of Docker test.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
122-123:⚠️ Potential issue | 🟡 MinorHEALTHCHECK will emit a shell error when
curl -ffails.When
sickchillis not yet responding,curl -fexits non-zero and the command substitution yields an empty string, turning the test into[ == "{}" ], which bash reports asunary operator expected(stderr noise, non-zero exit). Theelifbranch then probeshttps://localhost:8081, but SickChill on--port 8081is plain HTTP — that branch can never succeed. Net effect: the health check works once the service is up, but during startup it spams errors and the HTTPS fallback is unreachable.Consider quoting the substitution and dropping the unreachable HTTPS branch:
🔧 Proposed fix
HEALTHCHECK --interval=5m --timeout=3s \ - CMD bash -c 'if [ $(curl -f http://localhost:8081/ui/get_messages -s) == "{}" ]; then echo "sickchill is alive"; elif [ $(curl -f https://localhost:8081/ui/get_messages -s) == "{}" ]; then echo "sickchill is alive"; else echo 1; fi' + CMD bash -c '[ "$(curl -fs http://localhost:8081/ui/get_messages)" = "{}" ] && echo "sickchill is alive" || exit 1'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 122 - 123, The HEALTHCHECK line using bash -c with unquoted command substitutions and curl -f causes shell errors when curl fails and includes an unreachable HTTPS fallback; change the HEALTHCHECK command so the curl call to http://localhost:8081/ui/get_messages is captured into a quoted variable (use curl without -f or append || true to avoid non-zero exit), then test that variable with a quoted string comparison (e.g., [ "$output" = "{}" ]) and remove the https://localhost:8081 branch; update the HEALTHCHECK invocation that contains HEALTHCHECK ... CMD bash -c 'curl ... /ui/get_messages' accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 115-120: The Dockerfile currently defines duplicate CMD and EXPOSE
instructions so the logging-wrapper CMD (the bash -c "echo 'Starting
SickChill...' && sickchill --nolaunch --datadir /data --port 8081 || ...") is
overridden by the later exec-form CMD ["sickchill", "--nolaunch", "--datadir",
"/data", "--port", "8081"] and EXPOSE 8081 is declared twice; fix by keeping
only one EXPOSE 8081 and replacing the old exec-form CMD with the intended
behavior: either remove the logging wrapper and keep the exec-form CMD to
preserve proper signal handling, or remove the later exec-form CMD and EXPOSE so
the bash logging-wrapper becomes the sole CMD (if temporary debugging is
desired); update the Dockerfile to have a single CMD referencing sickchill and a
single EXPOSE 8081 accordingly.
- Line 9: The Dockerfile was switched to python:3.11-slim-bookworm but still
installs Bullseye-era packages (libffi7, libssl1.1) which don't exist on
Bookworm; update the apt package list used in the RUN apt-get install step (the
stanza that follows the FROM line) to use the Bookworm equivalents (replace
libffi7 → libffi8 and libssl1.1 → libssl3) or, better, remove hard-coded
ABI-versioned package names and install the generic packages (libffi-dev,
libssl-dev) so they resolve to the correct versions on Bookworm; ensure apt-get
update precedes install and keep the same cleanup (apt-get clean && rm -rf
/var/lib/apt/lists/*).
---
Outside diff comments:
In `@Dockerfile`:
- Around line 122-123: The HEALTHCHECK line using bash -c with unquoted command
substitutions and curl -f causes shell errors when curl fails and includes an
unreachable HTTPS fallback; change the HEALTHCHECK command so the curl call to
http://localhost:8081/ui/get_messages is captured into a quoted variable (use
curl without -f or append || true to avoid non-zero exit), then test that
variable with a quoted string comparison (e.g., [ "$output" = "{}" ]) and remove
the https://localhost:8081 branch; update the HEALTHCHECK invocation that
contains HEALTHCHECK ... CMD bash -c 'curl ... /ui/get_messages' accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Updated dependencies and healthcheck command in Dockerfile.
Updated the Dockerfile to modify APT sources and install necessary packages.
1 participant
Fixes # Docker Image Test
Proposed changes in this pull request:
PR is based on the DEVELOP branch
Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
Read contribution guide
Summary by CodeRabbit