Skip to content

Conversation

@m-jahn
Copy link
Member

@m-jahn m-jahn commented Nov 4, 2025

closes #44

Summary by CodeRabbit

  • New Features

    • Conditional Apptainer installation step added (includes PPA setup) for workflows.
    • Consolidated and improved containerization test step for CI.
  • Bug Fixes

    • Added input validation that returns clear error messages for invalid task values.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

📝 Walkthrough

Walkthrough

Updated action inputs' quote style, added task validation that errors on invalid values, and added a conditional "Install Apptainer" step (adds Apptainer PPA then installs) plus workflow adjustments to call the action with install-apptainer when containerizing.

Changes

Cohort / File(s) Change Summary
Action metadata and runtime
action.yml
Normalized default-value quoting to double quotes; added task validation step that prints an explicit error and exits on invalid task; added conditional "Install Apptainer" step which adds the Apptainer PPA then installs; adjusted step ordering and removed trailing blank line
Workflow invocation
.github/workflows/main.yml
Upgraded actions/checkout from v1 to v4; added/updated test steps to run the local action including a "Test run" and a consolidated "Test containerize" step passing install-apptainer: true and task: "containerize"; adjusted ancillary steps (e.g., show Dockerfile) for new flow

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant GH as GitHub Action
    participant V as Validator
    participant A as Apptainer Installer
    participant S as Snakemake Workflow

    User->>GH: dispatch with inputs (task, install-apptainer, ...)
    GH->>V: validate `task` input
    alt invalid task
        V-->>User: print error message
        GH-->>GH: exit non-zero
    else valid task
        V-->>GH: validation ok
        alt install-apptainer == true
            GH->>A: add Apptainer PPA && apt install apptainer
            A-->>GH: install result
            alt install failed
                GH-->>User: install error and exit
            end
        else install-apptainer == false
            GH-->>GH: skip apptainer install
        end
        GH->>S: run requested task (e.g., containerize)
        S-->>User: task completes
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas to focus during review:
    • action.yml — validate the exact conditional expressions and exit behavior for task validation.
    • action.yml — verify the PPA add-command and apt-install steps (order, sudo usage, idempotence).
    • .github/workflows/main.yml — ensure the upgraded checkout action and updated test steps correctly pass inputs and behave on runners.

Suggested reviewers

  • johanneskoester

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: add apptainer source, yml formatting' accurately reflects the main changes: adding Apptainer PPA source configuration and YAML formatting updates.
Linked Issues check ✅ Passed The PR successfully addresses issue #44 by introducing a new Install Apptainer step that adds the apptainer PPA repository before installation, resolving the Ubuntu package source issue.
Out of Scope Changes check ✅ Passed Changes are within scope: Apptainer PPA addition addresses issue #44, YAML formatting is supporting change, and GitHub Actions version upgrade is reasonable maintenance.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix_apptainer

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d438364 and 6e90b21.

📒 Files selected for processing (1)
  • .github/workflows/main.yml (1 hunks)
⏰ 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). (1)
  • GitHub Check: build
🔇 Additional comments (4)
.github/workflows/main.yml (4)

15-15: Good: checkout action upgraded to v4.

Using a current version of the checkout action is important for security and compatibility. v1 is significantly outdated.


17-22: New "Test run" step looks good.

The step correctly uses the local action with appropriate parameters to test the basic workflow functionality before containerization.


33-35: "Show Dockerfile" step looks good.

The step correctly references the generated Dockerfile for inspection after containerization.


24-31: All inputs are correctly defined and PPA installation logic is properly implemented.

Verification confirms:

  • install-apptainer input is defined in action.yml (line 37)
  • task input is defined in action.yml (line 31)
  • The action correctly adds the Apptainer PPA repository (sudo add-apt-repository -y ppa:apptainer/ppa, line 60) before installation, as required by the PR objectives

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
action.yml (1)

20-20: Improve YAML consistency with normalized quote styles.

The quote style normalization from single to double quotes across default values improves readability and YAML consistency. These are stylistic changes with no functional impact.

Also applies to: 24-24, 28-28, 36-36, 43-43

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4e2fd67 and d438364.

📒 Files selected for processing (1)
  • action.yml (2 hunks)
🔇 Additional comments (2)
action.yml (2)

45-50: Input validation with clear error messaging.

The new validation step is well-implemented. The conditional logic correctly identifies invalid task values and exits with explicit error messaging, improving user experience by providing actionable feedback early in the workflow. The condition ! (inputs.task == 'containerize' || inputs.task == 'run') properly triggers only when the task is neither of the two allowed values.


52-62: Apptainer installation step with proper safeguards.

The conditional Apptainer installation is well-structured:

  • Runs only when install-apptainer is explicitly enabled
  • Checks for apt-get availability before attempting installation, with clear error messaging for unsupported runners
  • Adds the PPA, updates the package cache, and installs Apptainer using standard Ubuntu commands
  • Proper use of -y flags for non-interactive operation in CI

Please verify that ppa:apptainer/ppa is the correct and currently active PPA for Apptainer on Ubuntu runners. If you'd like, I can search for the latest Apptainer installation documentation to confirm this is the recommended source.

@m-jahn
Copy link
Member Author

m-jahn commented Nov 11, 2025

note to revs: the file diff looks more complicated then it is -- the only line that was added and that is important is sudo add-apt-repository -y ppa:apptainer/ppa.

@johanneskoester
Copy link
Contributor

Thanks a lot!

@johanneskoester johanneskoester merged commit 73be4c1 into master Nov 12, 2025
4 checks passed
@johanneskoester johanneskoester deleted the fix_apptainer branch November 12, 2025 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

installation of apptainer does currently not work, as it's not on official package sources

3 participants