fix: containerize arguments#58
Conversation
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Action as GitHub Action
participant Snakemake
participant FS as Workspace
User->>Action: provides inputs (task, containerize-args, ...)
Action->>Snakemake: run `snakemake --containerize <containerize-args>`
Snakemake->>FS: write container recipe (`Dockerfile` or `apptainer.def`)
Snakemake-->>Action: exit status / artifacts
Action->>User: job completes, artifact available
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
23-25:⚠️ Potential issue | 🟡 MinorStale
taskdescription — now also produces Apptainer recipes.This section still describes the
containerizetask as generating "a container image specification (in the form of a Dockerfile)". With the newcontainerize-argssupport, the output can also be an Apptainer definition file (apptainer.def). Worth updating so the docs don't contradict the new input documented below.📝 Suggested wording
### `task` -Whether to run Snakemake or to generate a container image specification (in the form of a Dockerfile) that contains all required environments. Can be either `run` or `containerize`. Default `run`. +Whether to run Snakemake or to generate a container image specification (a `Dockerfile` or an Apptainer definition file, see `containerize-args`) that contains all required environments. Can be either `run` or `containerize`. Default `run`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 23 - 25, Update the `task` README description to reflect that `containerize` can produce either a Dockerfile or an Apptainer definition file by referencing the new `containerize-args` option; change the sentence that currently says "a container image specification (in the form of a Dockerfile)" to mention both "a Dockerfile or an Apptainer definition file (e.g. apptainer.def)" and keep the allowed values (`run` or `containerize`) and default (`run`) unchanged so the docs align with `containerize-args` behavior.
🧹 Nitpick comments (3)
README.md (1)
43-45: Aligncontainerize-argsdoc with the action input description.Same wording concerns as in
action.yml(see my comment there): "Additional arguments forcontainerize" understates the purpose, and listing''as a separate option confuses the default behavior. Also, this line is missing a trailing period while the surrounding input descriptions end in one.📝 Suggested wording
### `containerize-args` -Additional arguments for `containerize`, can be one of 'dockerfile', 'apptainer' or '' (defaults to dockerfile) +Selects the output format passed to `snakemake --containerize`. One of `dockerfile` or `apptainer`; if unset, Snakemake's default is used (currently `dockerfile`). The generated recipe is written to `Dockerfile` or `apptainer.def`, respectively.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 43 - 45, Update the README entry for the input named "containerize-args" to match the action input description: clarify that it selects which containerizer to use for containerize (not "additional arguments"), list the valid options as "dockerfile" or "apptainer" and state the default is "dockerfile" (do not list '' as a separate option), and add a trailing period to the sentence; reference the "containerize-args" input and the "containerize" action when making the change.action.yml (2)
124-130: Recipe selection is tied to an exact-literal match — consider normalizing.The logic is correct for the documented inputs, but the
== "apptainer"check is strictly literal. If a user passes"Apptainer"," apptainer", or any future alias that Snakemake may accept, the recipe file will silently be namedDockerfilewhile Snakemake emits an apptainer definition into it. Not a blocker (Snakemake's error behavior still applies when the value is outright invalid), but a small normalization makes this more robust:♻️ Optional hardening
run: | - if [[ "${{ inputs.containerize-args }}" == "apptainer" ]]; then + containerize_args="$(echo '${{ inputs.containerize-args }}' | tr '[:upper:]' '[:lower:]' | xargs)" + if [[ "$containerize_args" == "apptainer" ]]; then recipe="apptainer.def" else recipe="Dockerfile" fi - snakemake --directory ${{ inputs.directory }} --snakefile ${{ inputs.snakefile }} --show-failed-logs ${{ inputs.args }} --containerize ${{ inputs.containerize-args }} > $recipe + snakemake --directory ${{ inputs.directory }} --snakefile ${{ inputs.snakefile }} --show-failed-logs ${{ inputs.args }} --containerize $containerize_args > "$recipe"Also worth noting: when
inputs.containerize-argsis empty, the command expands to a bare--containerize(no argument), which relies on Snakemake's argparsenargs='?'default. That works today but is implicit — omitting the flag entirely when the input is empty would be more defensive.Error handling in this step is intentionally omitted, which aligns with prior feedback that Snakemake itself surfaces proper containerization errors. Based on learnings from PR
#39, additional error handling here is unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@action.yml` around lines 124 - 130, Normalize the inputs.containerize-args value before comparing/using it and only append the --containerize flag when it is non-empty: trim whitespace and lowercase the value (so variants like " Apptainer" or "APPTAINER" match) and then set recipe="apptainer.def" when the normalized value equals "apptainer"; otherwise set recipe="Dockerfile". Also conditionally include the --containerize argument in the snakemake invocation only when the original/normalized inputs.containerize-args is non-empty to avoid emitting a bare --containerize flag.
48-51: Tighten thecontainerize-argsdescription.The current description is misleading. It says "Additional arguments for --containerize," but this input doesn't add extra arguments—it selects the output format for the
--containerizeflag. Also, listing''alongside'dockerfile'and'apptainer'obscures the fact that empty means "use Snakemake's default" (currentlydockerfile). If Snakemake changes its default in a future version, the hardcodedDockerfilefallback at line 128 would silently produce a mismatched output.📝 Proposed wording
containerize-args: - description: Additional arguments for --containerize, can be one of 'dockerfile', 'apptainer' or '' (defaults to dockerfile) + description: | + Output format to pass to Snakemake's --containerize flag. + One of 'dockerfile' or 'apptainer'. If empty, Snakemake's own default is used + (currently 'dockerfile'). Selects the recipe filename accordingly + (Dockerfile or apptainer.def). required: false default: ""Per Snakemake's CLI documentation,
--containerizesupports onlydockerfile(default) andapptainer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@action.yml` around lines 48 - 51, The input description for "containerize-args" is misleading: update it to say this input selects the --containerize output format (allowed values: "dockerfile" or "apptainer") and that an empty string means "use Snakemake's default" (not an additional argument); remove the misleading listing of '' as a third explicit option and clarify the default behavior while keeping default: ""; also update any related doc comment referencing the hardcoded Dockerfile fallback so it notes that the fallback is only used when we explicitly override and not assumed when the input is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@README.md`:
- Around line 23-25: Update the `task` README description to reflect that
`containerize` can produce either a Dockerfile or an Apptainer definition file
by referencing the new `containerize-args` option; change the sentence that
currently says "a container image specification (in the form of a Dockerfile)"
to mention both "a Dockerfile or an Apptainer definition file (e.g.
apptainer.def)" and keep the allowed values (`run` or `containerize`) and
default (`run`) unchanged so the docs align with `containerize-args` behavior.
---
Nitpick comments:
In `@action.yml`:
- Around line 124-130: Normalize the inputs.containerize-args value before
comparing/using it and only append the --containerize flag when it is non-empty:
trim whitespace and lowercase the value (so variants like " Apptainer" or
"APPTAINER" match) and then set recipe="apptainer.def" when the normalized value
equals "apptainer"; otherwise set recipe="Dockerfile". Also conditionally
include the --containerize argument in the snakemake invocation only when the
original/normalized inputs.containerize-args is non-empty to avoid emitting a
bare --containerize flag.
- Around line 48-51: The input description for "containerize-args" is
misleading: update it to say this input selects the --containerize output format
(allowed values: "dockerfile" or "apptainer") and that an empty string means
"use Snakemake's default" (not an additional argument); remove the misleading
listing of '' as a third explicit option and clarify the default behavior while
keeping default: ""; also update any related doc comment referencing the
hardcoded Dockerfile fallback so it notes that the fallback is only used when we
explicitly override and not assumed when the input is empty.
In `@README.md`:
- Around line 43-45: Update the README entry for the input named
"containerize-args" to match the action input description: clarify that it
selects which containerizer to use for containerize (not "additional
arguments"), list the valid options as "dockerfile" or "apptainer" and state the
default is "dockerfile" (do not list '' as a separate option), and add a
trailing period to the sentence; reference the "containerize-args" input and the
"containerize" action when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f691b240-fd76-4563-94a6-d9719630934f
📒 Files selected for processing (3)
LICENSEREADME.mdaction.yml
|
Just tested the fixed workflow with our automated Apptainer Deployment action ... and it worked nicely. |
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 `@README.md`:
- Around line 43-46: The docs and metadata use the invalid value 'dockerfile'
for containerize-args; update the occurrences of the containerize-args
description in README.md and action.yml to state allowed values 'docker' or
'apptainer' (or empty) so that the value passed to snakemake --containerize
matches Snakemake's expected 'docker' token; search for the identifier
containerize-args and change 'dockerfile' to 'docker' in both README and
action.yml and ensure any examples that pass containerize-args to snakemake
--containerize use 'docker' not 'dockerfile'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores