Ci/apptainer on release #423
Conversation
WalkthroughA new GitHub Actions workflow was introduced to build and attach an Apptainer image to releases. The rabies parser script updated the default for an analysis argument from a NIfTI to a CSV file. The DSURQE installation script improved its download logic with a fallback mechanism and enhanced error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Release Event
participant GitHub Actions Runner
participant Apptainer
participant GitHub Container Registry
participant GitHub Release Assets
GitHub Release Event->>GitHub Actions Runner: Trigger workflow on release
GitHub Actions Runner->>GitHub Actions Runner: Checkout code
GitHub Actions Runner->>GitHub Actions Runner: Install Apptainer
GitHub Actions Runner->>GitHub Actions Runner: Extract version info
GitHub Actions Runner->>Apptainer: Build image from Docker (via GHCR)
Apptainer-->>GitHub Actions Runner: Return .sif image
GitHub Actions Runner->>GitHub Release Assets: Upload .sif as release asset
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
this will be patch release when merged (0.5.2) |
|
This looks good, but I wonder is it worthwhile to append this to the end of the docker build, so it can pull directly from the local docker daemon with an already built image? |
| - name: Build Apptainer image from Docker URL | ||
| run: | | ||
| REPO_LOWERCASE=$(echo "${{ github.repository }}" | tr '[:upper:]' '[:lower:]') | ||
| sudo apptainer build app-${{ steps.get_version.outputs.VERSION }}.sif docker://ghcr.io/${REPO_LOWERCASE}:latest |
There was a problem hiding this comment.
can we use a tag here instead of :latest?
|
@gdevenyi i have it separate right now as it only triggers on a manual release. i don't think we want it on every build, right? |
CMonnin
force-pushed
the
ci/apptainer-on-release
branch
from
21c0e09 to
ce92458
Compare
Right. This is over optimization anyways. |
|
We have a new release, is this ready for merge and use @CMonnin |
CMonnin
force-pushed
the
ci/apptainer-on-release
branch
from
06e7df2 to
cd177c5
Compare
…llcheck formating on install_DSURQE.sh
|
rabased, should be good to go. moved from draft to PR |
|
The build here throws an error, any idea why? The last tagged build succeeded. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/apptainer-attach-on-release.yml(1 hunks)rabies/parser.py(3 hunks)scripts/install_DSURQE.sh(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/apptainer-attach-on-release.yml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
25-25: shellcheck reported issue in this script: SC2086:info:1:44: Double quote to prevent globbing and word splitting
(shellcheck)
28-28: shellcheck reported issue in this script: SC2086:info:3:58: Double quote to prevent globbing and word splitting
(shellcheck)
34-34: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (7)
.github/workflows/apptainer-attach-on-release.yml (1)
1-39: Well-structured workflow implementation.The workflow correctly implements the automated Apptainer image building and attachment process as described in the PR objectives. The logic flow is sound and the separation from the main Docker build process is appropriate for manual releases only.
scripts/install_DSURQE.sh (3)
8-44: Excellent implementation of fallback download strategy.The two-tiered download approach with primary (mouseimaging.ca) and fallback (GitHub) sources is well-designed. The logic correctly attempts primary downloads first, detects failures, and falls back to compressed versions when needed.
40-40: Good error handling with curl options.The curl commands now include proper flags for robust downloading:
--failfor HTTP error detection,--silent --show-errorfor clean output, and--retry 5for resilience.Also applies to: 53-53
62-72: Good implementation of proper quoting.All curl commands now properly quote URLs and output paths, addressing the previous shellcheck recommendations. This prevents issues with filenames containing spaces or special characters.
rabies/parser.py (3)
2-4: Minor import organization improvement.The reordering of imports is a good practice for code organization and readability.
959-959: Good alignment with install script changes.The default value change from NIfTI labels to CSV mapping file correctly aligns with the updated
install_DSURQE.shscript that now prioritizes downloading the CSV mapping file. This ensures consistency between the expected input format and what the installation process provides.
1201-1201: Minor formatting improvement.The blank line addition before the import improves code readability.
scripts/install_DSURQE.sh
Outdated
| if [ "$primary_success" = true ]; then | ||
| gzip -f ${out_dir}/DSURQE_40micron_*.nii | ||
| fi |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify gzip compression behavior.
The script compresses .nii files only after successful primary downloads, but the compression command lacks proper quoting and may fail with certain filenames.
Run the following script to verify the gzip command handles filenames correctly:
🏁 Script executed:
#!/bin/bash
# Description: Check if gzip command properly handles filenames with spaces or special characters
# Expected: The command should be quoted to handle edge cases
# Search for gzip usage in the script
rg -A 2 -B 2 "gzip.*DSURQE" scripts/install_DSURQE.shLength of output: 142
Quote variable expansions in the gzip command
Unquoted expansions can break on filenames containing spaces or special characters. Update the invocation in scripts/install_DSURQE.sh (around lines 46–48) to properly quote the directory and handle each file safely. For example:
if [ "$primary_success" = true ]; then
- gzip -f ${out_dir}/DSURQE_40micron_*.nii
+ # Quote the directory and iterate to handle spaces/special chars
+ for f in "${out_dir}"/DSURQE_40micron_*.nii; do
+ gzip -f "$f"
+ done
fi- Location: scripts/install_DSURQE.sh lines 46–48
- Change: wrap ${out_dir} in quotes and gzip each file in a loop to avoid word-splitting issues.
📝 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.
| if [ "$primary_success" = true ]; then | |
| gzip -f ${out_dir}/DSURQE_40micron_*.nii | |
| fi | |
| if [ "$primary_success" = true ]; then | |
| # Quote the directory and iterate to handle spaces/special chars | |
| for f in "${out_dir}"/DSURQE_40micron_*.nii; do | |
| gzip -f "$f" | |
| done | |
| fi |
🤖 Prompt for AI Agents
In scripts/install_DSURQE.sh around lines 46 to 48, the gzip command uses an
unquoted variable expansion which can break if filenames contain spaces or
special characters. To fix this, wrap the ${out_dir} variable in double quotes
and iterate over each matching file in a loop, applying gzip individually to
safely handle each filename without word-splitting issues.
SSL cert issues with https://www.mouseimaging.ca |
|
ok the SSL cert error is there but the fallback was actually working as intended (just silently). Re-running the build worked. Not sure why it failed now . . . |
|
Can you address the bot requests |
|
bot addressed, builds successful on 1st attempt |
testing on my fork
release
action
Summary by CodeRabbit
New Features
Bug Fixes
Other Changes