Skip to content

Updates (new images, instructions)#5

Merged
ypriverol merged 21 commits intobigbio:devfrom
biosustain:main
Apr 9, 2026
Merged

Updates (new images, instructions)#5
ypriverol merged 21 commits intobigbio:devfrom
biosustain:main

Conversation

@enryH
Copy link
Copy Markdown
Collaborator

@enryH enryH commented Mar 24, 2026

Summary by CodeRabbit

  • Chores
    • Standardized registry and image naming via workflow-level variables.
    • Refined CI flow to separately build/publish Docker, convert to Singularity, and deploy versioned/latest tags, with pushes controlled by inputs.
    • Added retries, per-build success/failure notifications, and stricter job gating for downstream runs.
    • Updated workflow permissions and credential handling to improve publishing reliability and deployment security.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 24, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Added workflow-level registry/image envs; refactored build-diann into discrete Docker metadata, conditional Docker build/push, pull/save, Singularity conversion and push; switched Singularity auth to github.actor + secrets.GITHUB_TOKEN; added retries, expanded permissions, and per-matrix notifications; tightened downstream owner gating.

Changes

Cohort / File(s) Summary
GitHub Actions workflow
.github/workflows/quantms-containers.yml
Introduced workflow env values (REGISTRY, IMAGE_NAME, SINGULARITY_IMAGE_NAME); replaced hardcoded bigbio references with github.repository_owner in image names; refactored build-diann into metadata extraction, conditional Docker build/push (driven by github.event.inputs.push_images), separate Docker pull/save, Singularity setup/convert/push; added retry envs (RETRY_TIMES, RETRY_DELAY), expanded job permissions (attestations: write, id-token: write), added step-level success/failure notifications, and changed Singularity credential usage to github.actor + secrets.GITHUB_TOKEN; tightened if: for build-relink and sync-openms to github.repository_owner == 'bigbio'.

Sequence Diagram(s)

sequenceDiagram
    participant Actions as GitHub Actions
    participant Meta as docker/metadata-action
    participant Docker as Docker Build/Push
    participant Registry as Container Registry
    participant Runner as Actions Runner (pull/save)
    participant Singularity as Singularity Build/Push
    participant Notifier as Notification Steps

    Actions->>Meta: generate image metadata
    Meta->>Docker: provide tags & build inputs
    Docker->>Registry: conditional push (if push_images)
    Registry->>Actions: image available
    Actions->>Runner: pull image & save tar (if push_images)
    Runner->>Singularity: singularity build from tar -> .sif
    Singularity->>Registry: singularity remote login & push (version, latest)
    Registry->>Actions: push confirmation
    Actions->>Notifier: matrix success/failure notifications
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Add diann 232 #6 — Overlapping edits to quantms-containers.yml: image envs, DIANN build/singularity flow, and switching credentials to github.actor/GITHUB_TOKEN.

Poem

🐰 I hopped through YAML with a cheerful spin,

Tags set tidy, tokens tucked within,
Docker hummed and Singularity grinned,
Retries braced me for each build win,
I munched a carrot as CI took a spin.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Updates (new images, instructions)' is vague and generic; it does not clearly describe the primary changes (workflow refactoring, CI/Docker improvements, credential handling). Revise the title to be more specific, such as 'Refactor quantms-containers workflows for generic registry and conditional image publishing' or similar, to better convey the actual scope of changes.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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
Copy Markdown

@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: 2

🧹 Nitpick comments (4)
.github/workflows/quantms-containers.yml (4)

64-71: Metadata step output appears unused.

The docker/metadata-action extracts tags into steps.meta.outputs.tags, but the build step (lines 82-84) defines its own tags directly instead of using ${{ steps.meta.outputs.tags }}. Either use the metadata output or remove this step if it's not needed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/quantms-containers.yml around lines 64 - 71, The
metadata-action step with id "meta" (docker/metadata-action@v5) produces tags in
steps.meta.outputs.tags but those outputs are not used; either remove the entire
"meta" step or wire its output into the image build step by replacing the
hard-coded tags with the metadata output (use steps.meta.outputs.tags) so the
build step consumes the generated tags; update references to "meta" and
"steps.meta.outputs.tags" accordingly.

40-44: Consider if attestations: write and id-token: write permissions are needed.

These permissions are typically required for SLSA provenance attestations, but the build step has provenance: false (line 87). If attestations aren't planned, these permissions could be removed following the principle of least privilege.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/quantms-containers.yml around lines 40 - 44, The workflow
currently grants permissions attestations: write and id-token: write although
the build step sets provenance: false; remove the unnecessary permissions by
deleting the attestations: write and id-token: write entries from the
permissions block (leaving packages, contents as-is) or change them to
least-privilege values, and ensure no other steps require SLSA provenance or
OIDC tokens before removing; check for any references to provenance or OIDC
usages elsewhere in the workflow to confirm it's safe to drop these keys.

116-116: Use env.REGISTRY for consistency.

The login command uses a hardcoded oras://ghcr.io while the push commands on lines 119 and 123 use ${{ env.REGISTRY }}. For maintainability, use the environment variable consistently.

♻️ Proposed fix
-        echo ${{ secrets.GITHUB_TOKEN }} | singularity remote login -u ${{ github.actor }} --password-stdin oras://ghcr.io
+        echo ${{ secrets.GITHUB_TOKEN }} | singularity remote login -u ${{ github.actor }} --password-stdin oras://${{ env.REGISTRY }}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/quantms-containers.yml at line 116, Replace the hardcoded
registry URI in the Singularity login step with the environment variable used
elsewhere; change the echo/login invocation that currently targets
"oras://ghcr.io" so it uses ${{ env.REGISTRY }} instead, ensuring the
singularity remote login command (the echo ${{ secrets.GITHUB_TOKEN }} |
singularity remote login -u ${{ github.actor }} --password-stdin ...) points at
the same REGISTRY variable used by later push steps.

122-123: Redundant condition check.

The condition "${{ env.IS_RELEASE }}" == "true" || "${{ github.event_name }}" == "release" is redundant since IS_RELEASE is derived from github.event_name == 'release'. Use one or the other.

Also note that on release events, both matrix jobs will push their SIF to :latest, resulting in the same race condition mentioned for Docker images.

♻️ Proposed simplification
-        if [[ "${{ env.IS_RELEASE }}" == "true" || "${{ github.event_name }}" == "release" ]]; then
+        if [[ "${{ env.IS_RELEASE }}" == "true" ]]; then
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/quantms-containers.yml around lines 122 - 123, The
conditional uses both env.IS_RELEASE and github.event_name redundantly
(env.IS_RELEASE is derived from github.event_name) and both matrix jobs push a
SIF to the same :latest tag causing a race; simplify the test to use one source
(e.g., use only env.IS_RELEASE or only github.event_name) and change the push to
use a unique tag to avoid races (e.g., tag with matrix.version or push :latest
only from a single designated matrix entry); update the singularity push line
(the singularity push command referencing matrix.version and
env.SINGULARITY_IMAGE_NAME) accordingly so each job pushes a uniquely tagged
image or only the designated job pushes :latest.
🤖 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/quantms-containers.yml:
- Line 81: The workflow uses boolean comparisons against
github.event.inputs.push_images which are strings, so change the conditional
expressions that reference the push: ${{ ... }} lines to compare the input as a
string (e.g., compare github.event.inputs.push_images != 'false' or check ==
'true' as a string) instead of comparing to a boolean; update all occurrences
that reference github.event.inputs.push_images (the push condition lines shown
in the diff and the other similar conditions mentioned) to use the string
comparison so workflow_dispatch inputs behave correctly.
- Around line 82-86: The workflow currently always emits a :latest tag for every
matrix.version, causing non-deterministic overwrites; remove the unconditional
latest entry from the tags block (keep "${{ env.REGISTRY }}/${{ env.IMAGE_NAME
}}:${{ matrix.version }}") and add a separate step that pushes or tags the image
as :latest only when matrix.version matches the desired version (e.g., use an
additional job/step with if: matrix.version == '2.1.0' that tags/pushes "${{
env.REGISTRY }}/${{ env.IMAGE_NAME }}:latest"); reference matrix.version and the
tags field to locate and update the code.

---

Nitpick comments:
In @.github/workflows/quantms-containers.yml:
- Around line 64-71: The metadata-action step with id "meta"
(docker/metadata-action@v5) produces tags in steps.meta.outputs.tags but those
outputs are not used; either remove the entire "meta" step or wire its output
into the image build step by replacing the hard-coded tags with the metadata
output (use steps.meta.outputs.tags) so the build step consumes the generated
tags; update references to "meta" and "steps.meta.outputs.tags" accordingly.
- Around line 40-44: The workflow currently grants permissions attestations:
write and id-token: write although the build step sets provenance: false; remove
the unnecessary permissions by deleting the attestations: write and id-token:
write entries from the permissions block (leaving packages, contents as-is) or
change them to least-privilege values, and ensure no other steps require SLSA
provenance or OIDC tokens before removing; check for any references to
provenance or OIDC usages elsewhere in the workflow to confirm it's safe to drop
these keys.
- Line 116: Replace the hardcoded registry URI in the Singularity login step
with the environment variable used elsewhere; change the echo/login invocation
that currently targets "oras://ghcr.io" so it uses ${{ env.REGISTRY }} instead,
ensuring the singularity remote login command (the echo ${{ secrets.GITHUB_TOKEN
}} | singularity remote login -u ${{ github.actor }} --password-stdin ...)
points at the same REGISTRY variable used by later push steps.
- Around line 122-123: The conditional uses both env.IS_RELEASE and
github.event_name redundantly (env.IS_RELEASE is derived from github.event_name)
and both matrix jobs push a SIF to the same :latest tag causing a race; simplify
the test to use one source (e.g., use only env.IS_RELEASE or only
github.event_name) and change the push to use a unique tag to avoid races (e.g.,
tag with matrix.version or push :latest only from a single designated matrix
entry); update the singularity push line (the singularity push command
referencing matrix.version and env.SINGULARITY_IMAGE_NAME) accordingly so each
job pushes a uniquely tagged image or only the designated job pushes :latest.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4bde26ce-cbed-4d18-8f4d-24f9b5d838bb

📥 Commits

Reviewing files that changed from the base of the PR and between 6c75c29 and dee53ed.

📒 Files selected for processing (1)
  • .github/workflows/quantms-containers.yml

Comment thread .github/workflows/quantms-containers.yml Outdated
Comment thread .github/workflows/quantms-containers.yml Outdated
@ypriverol ypriverol self-requested a review March 24, 2026 13:40
Copy link
Copy Markdown

@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: 2

♻️ Duplicate comments (2)
.github/workflows/quantms-containers.yml (2)

86-88: ⚠️ Potential issue | 🟡 Minor

All matrix versions overwrite the :latest tag non-deterministically.

This issue was flagged in a previous review and remains unresolved. All 5 versions push to :latest, so the final tag depends on which job finishes last.

💡 Proposed fix: Only tag latest for the newest version
         tags: |
           ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:${{ matrix.version }}
-          ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:latest
+          ${{ matrix.version == '2.2.0' && format('{0}/{1}:latest', env.REGISTRY, env.IMAGE_NAME) || '' }}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/quantms-containers.yml around lines 86 - 88, The tags
block currently makes every matrix job push both ${ env.REGISTRY }/${
env.IMAGE_NAME }:${ matrix.version } and :latest, causing races; change the
workflow so only the newest version job pushes the :latest tag—e.g., add a
condition that compares matrix.version to the designated newest version (or
introduce a matrix field like latest: true for the newest entry) and only emit
the :latest tag when that condition is true; update the tags entries that
reference ${ matrix.version } and the :latest entry to be conditional so only
the job where matrix.version (or matrix.latest) indicates the newest version
includes the :latest tag.

85-85: ⚠️ Potential issue | 🟠 Major

Boolean comparison with github.event.inputs does not work as expected.

This issue was flagged in a previous review and remains unresolved. github.event.inputs.push_images is always a string ("true" or "false"), so comparing to boolean true always evaluates to false.

Current behavior:

  • workflow_dispatch: push is always false regardless of input (broken)
  • push/release/pull_request: push is always true (PRs probably shouldn't push)
🐛 Proposed fix
-        push: ${{ (github.event.inputs.push_images == true || github.event.inputs.push_images == '') }}
+        push: ${{ github.event_name != 'pull_request' && github.event.inputs.push_images != 'false' }}

This ensures:

  • PRs never push
  • workflow_dispatch respects the input toggle
  • push/release events push by default
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/quantms-containers.yml at line 85, The push condition uses
a boolean comparison against github.event.inputs.push_images (a string), so it
mis-evaluates; update the push expression to compare the string value and also
prevent PRs from pushing, e.g. replace the current push: ${{
(github.event.inputs.push_images == true || github.event.inputs.push_images ==
'') }} with a string-aware expression that checks github.event_name and the
string value of github.event.inputs.push_images (for example: push: ${{
(github.event_name == 'workflow_dispatch' && github.event.inputs.push_images ==
'true') || (github.event_name != 'workflow_dispatch' && github.event_name !=
'pull_request') }}), using github.event.inputs.push_images, github.event_name
and the push key to locate and update the condition.
🧹 Nitpick comments (3)
.github/workflows/quantms-containers.yml (3)

68-79: Metadata and date tag are computed but never used.

The meta step extracts Docker metadata tags, and the date step computes DATE_TAG, but neither output is referenced in the workflow. The build step uses hardcoded tags instead.

Either remove these unused steps or update the build step to use the metadata outputs.

🧹 Option 1: Remove unused steps
-    - name: Extract metadata for Docker
-      id: meta
-      uses: docker/metadata-action@v5
-      with:
-        images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}
-        tags: |
-          type=raw,value=latest,enable={{is_default_branch}}
-          type=raw,value={{date 'YYYY.MM.DD'}}
-
-    - name: Set date tag
-      id: date
-      run: echo "DATE_TAG=$(date +'%Y.%m.%d')" >> $GITHUB_OUTPUT
-
     - name: Build and Push DiaNN ${{ matrix.version }} image (Latest)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/quantms-containers.yml around lines 68 - 79, The workflow
currently computes Docker metadata in the step with id "meta"
(docker/metadata-action@v5) and a date in the step with id "date" but neither
output is used; either remove these steps or wire their outputs into the image
build/push step: reference the metadata-action outputs (e.g., the tags from
steps.meta.outputs) and the computed date (from steps.date.outputs.DATE_TAG) in
the build/push step instead of hardcoded tags so the produced images use the
generated tags and date; locate the "meta" and "date" steps and the build/push
step in the workflow and update the build step to consume steps.meta.outputs.*
and steps.date.outputs.DATE_TAG or delete the unused steps.

34-41: Consider adding fail-fast: false to the matrix strategy.

Currently, if one version fails to build, GitHub Actions cancels all other matrix jobs by default. Since these container versions are independent, you may want to allow successful builds to complete even if one fails.

💡 Proposed change
     strategy:
+      fail-fast: false
       matrix:
         version:
           - 2.2.0
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/quantms-containers.yml around lines 34 - 41, The
workflow's matrix strategy currently defaults to cancelling other jobs when one
fails; add a `fail-fast: false` setting under the `strategy` block (alongside
`matrix`) to ensure independent `version` matrix jobs (the listed versions
2.2.0, 2.1.0, 2.0.2, 1.9.2, 1.8.1) are allowed to finish even if one fails.

50-52: Unused environment variables.

RETRY_TIMES and RETRY_DELAY are defined but never used in this workflow. Consider removing them or implementing the retry logic if it was intended.

🧹 Proposed fix to remove unused variables
-    env:
-      RETRY_TIMES: 3
-      RETRY_DELAY: 30
-
     steps:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/quantms-containers.yml around lines 50 - 52, The env
variables RETRY_TIMES and RETRY_DELAY are defined but unused; either remove
these two entries from the env block to clean up the workflow, or implement
actual retry behavior by wiring RETRY_TIMES and RETRY_DELAY into the relevant
job/step(s) (e.g., wrap the failing command in a shell loop that uses
$RETRY_TIMES and sleeps $RETRY_DELAY between attempts, or use a retry action) so
the variables are consumed; look for the env block containing RETRY_TIMES and
RETRY_DELAY to apply the change.
🤖 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/quantms-containers.yml:
- Around line 115-128: The "Login and Deploy Container" step currently gates
Singularity pushes only on a non-PR check and inconsistently tags :latest;
update this step to use the same push control as Docker by checking the workflow
input push_images (the push_images input used by the Docker step) and wrap the
release check with the existing env.IS_RELEASE variable for consistency, then
mirror Docker's tagging behavior by always pushing the version tag and also
pushing :latest when push_images is enabled (and when IS_RELEASE is true if you
want to keep release-only latest) so the singularity push commands (singularity
push diann-${{ matrix.version }}.sif ...) follow the same conditions and :latest
semantics as the Docker pushes.

---

Duplicate comments:
In @.github/workflows/quantms-containers.yml:
- Around line 86-88: The tags block currently makes every matrix job push both
${ env.REGISTRY }/${ env.IMAGE_NAME }:${ matrix.version } and :latest, causing
races; change the workflow so only the newest version job pushes the :latest
tag—e.g., add a condition that compares matrix.version to the designated newest
version (or introduce a matrix field like latest: true for the newest entry) and
only emit the :latest tag when that condition is true; update the tags entries
that reference ${ matrix.version } and the :latest entry to be conditional so
only the job where matrix.version (or matrix.latest) indicates the newest
version includes the :latest tag.
- Line 85: The push condition uses a boolean comparison against
github.event.inputs.push_images (a string), so it mis-evaluates; update the push
expression to compare the string value and also prevent PRs from pushing, e.g.
replace the current push: ${{ (github.event.inputs.push_images == true ||
github.event.inputs.push_images == '') }} with a string-aware expression that
checks github.event_name and the string value of github.event.inputs.push_images
(for example: push: ${{ (github.event_name == 'workflow_dispatch' &&
github.event.inputs.push_images == 'true') || (github.event_name !=
'workflow_dispatch' && github.event_name != 'pull_request') }}), using
github.event.inputs.push_images, github.event_name and the push key to locate
and update the condition.

---

Nitpick comments:
In @.github/workflows/quantms-containers.yml:
- Around line 68-79: The workflow currently computes Docker metadata in the step
with id "meta" (docker/metadata-action@v5) and a date in the step with id "date"
but neither output is used; either remove these steps or wire their outputs into
the image build/push step: reference the metadata-action outputs (e.g., the tags
from steps.meta.outputs) and the computed date (from
steps.date.outputs.DATE_TAG) in the build/push step instead of hardcoded tags so
the produced images use the generated tags and date; locate the "meta" and
"date" steps and the build/push step in the workflow and update the build step
to consume steps.meta.outputs.* and steps.date.outputs.DATE_TAG or delete the
unused steps.
- Around line 34-41: The workflow's matrix strategy currently defaults to
cancelling other jobs when one fails; add a `fail-fast: false` setting under the
`strategy` block (alongside `matrix`) to ensure independent `version` matrix
jobs (the listed versions 2.2.0, 2.1.0, 2.0.2, 1.9.2, 1.8.1) are allowed to
finish even if one fails.
- Around line 50-52: The env variables RETRY_TIMES and RETRY_DELAY are defined
but unused; either remove these two entries from the env block to clean up the
workflow, or implement actual retry behavior by wiring RETRY_TIMES and
RETRY_DELAY into the relevant job/step(s) (e.g., wrap the failing command in a
shell loop that uses $RETRY_TIMES and sleeps $RETRY_DELAY between attempts, or
use a retry action) so the variables are consumed; look for the env block
containing RETRY_TIMES and RETRY_DELAY to apply the change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8ef14f32-d99f-42ec-a0ed-127873e8deea

📥 Commits

Reviewing files that changed from the base of the PR and between dee53ed and 14d827a.

📒 Files selected for processing (1)
  • .github/workflows/quantms-containers.yml

Comment thread .github/workflows/quantms-containers.yml Outdated
Comment thread .github/workflows/quantms-containers.yml Outdated
@ypriverol
Copy link
Copy Markdown
Member

@enryH Can you check.

@enryH
Copy link
Copy Markdown
Collaborator Author

enryH commented Mar 24, 2026

I will discuss it with Dominik tomorrow:

 1 warning found (use docker --debug to expand):
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 19)
ERROR: failed to build: failed to solve: failed to push ghcr.io/bigbio/diann:1.8.1: denied: installation not allowed to Write organization package
Error: buildx failed with: ERROR: failed to build: failed to solve: failed to push ghcr.io/bigbio/diann:1.8.1: denied: installation not allowed to Write organization package

@Skitionek
Copy link
Copy Markdown

My work was focusing on easy/fast way to have it running for us, as simplification I have deleted part of original logic (example relink is not build). Since the forking update to original repos was substantial, @ypriverol did some good updates which aligns with some of my refractors.
I would suggest to put it into draft as it needs rework so no functionality is being deleted.

@ypriverol
Copy link
Copy Markdown
Member

Should I just close this PR for now?

@enryH
Copy link
Copy Markdown
Collaborator Author

enryH commented Mar 25, 2026

I think I would give it a try to align your changes with our. Can you explain what relink is for?

enryH added 3 commits March 25, 2026 11:59
- only run on modification (detect-changes)
- relink workflow
- openms-sync step

Relink and open-ms step could be set to run only if the owner is bigbio
Copy link
Copy Markdown

@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: 2

🤖 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/quantms-containers.yml:
- Around line 104-116: The job title and many uses rely on matrix.version but
the matrix produced by needs.detect-changes.outputs.diann_matrix lacks a version
field; update the detect-changes matrix generator so each diann_matrix entry
includes a "version" key (e.g., set version to the existing tag or extract it
from context) so that ${{
matrix.version }} is populated for the "Build and Push DiaNN ${{ matrix.version
}} Docker Images" job; ensure the diann_matrix JSON emitted by the
detect-changes job (needs.detect-changes.outputs.diann_matrix) contains version
for every element.
- Around line 130-135: Multiple publish steps (docker login/push and singularity
push in the build-diann, build-relink and sync-openms jobs) rely on
secrets.GITHUB_TOKEN which may not have organization package write permissions;
update the affected steps that call docker/login-action@v3 and the Singularity
push steps to consistently use a token with org package write permissions (e.g.
secrets.GHCR_TOKEN or a dedicated org PAT) instead of secrets.GITHUB_TOKEN, and
ensure build-relink does not mix GHCR_TOKEN for Docker and GITHUB_TOKEN for
Singularity—replace the registry login/password inputs to use the chosen secret
and verify ghcr.io/bigbio/* pushes reference that same secret across the docker
login and singularity push steps.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 17e1f702-7dde-4ed3-b905-9fd599b98733

📥 Commits

Reviewing files that changed from the base of the PR and between 14d827a and 4c83cd7.

📒 Files selected for processing (1)
  • .github/workflows/quantms-containers.yml

Comment thread .github/workflows/quantms-containers.yml Outdated
Comment thread .github/workflows/quantms-containers.yml Outdated
Copy link
Copy Markdown

@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: 4

🤖 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/quantms-containers.yml:
- Line 213: The if condition string `always() &&
needs.detect-changes.outputs.has_relink == 'true'&& github.repository_owner ==
'bigbio'` is missing a space before the `&&`; update the condition in the
workflow (the `if:` expression) to add spacing around the logical operator
(e.g., place a space before and after `&&`) so it reads clearly and
consistently.
- Around line 137-148: The metadata extraction step (id: meta) and the date step
(id: date which sets DATE_TAG) are producing outputs (steps.meta.outputs.tags,
steps.meta.outputs.labels and DATE_TAG) that are never consumed by the Docker
build step; either remove these unused steps or update the build step to use
them (replace the hardcoded image tags with steps.meta.outputs.tags and/or the
DATE_TAG output or include steps.meta.outputs.labels as build-args/labels).
Locate the actions by their step ids "meta" and "date" and the Docker build step
and make the change so the extracted metadata/date are actually passed to docker
build or delete the dead steps.
- Around line 118-121: Remove the unused environment variables RETRY_TIMES and
RETRY_DELAY from the build-diann job's env block: they are defined but never
consumed. Edit the job definition that contains the env: block defining
RETRY_TIMES and RETRY_DELAY and delete those two entries (or alternatively
implement retry logic where they're intended to be used if required), ensuring
no other steps reference RETRY_TIMES or RETRY_DELAY after the change.
- Around line 111-112: Remove the unnecessary permissions from the workflow
permissions block by deleting the "attestations: write" and "id-token: write"
entries so the job only requests required scopes; update the permissions stanza
that currently contains "attestations: write" and "id-token: write" to exclude
those keys (the Docker build already sets provenance: false and the build-relink
job omits them), ensuring the job follows least-privilege.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 600006cd-4134-4b8b-8799-255cc9589c6b

📥 Commits

Reviewing files that changed from the base of the PR and between 4c83cd7 and e0e9688.

📒 Files selected for processing (1)
  • .github/workflows/quantms-containers.yml

Comment thread .github/workflows/quantms-containers.yml Outdated
Comment thread .github/workflows/quantms-containers.yml Outdated
Comment thread .github/workflows/quantms-containers.yml Outdated
Comment thread .github/workflows/quantms-containers.yml Outdated
Comment on lines +68 to +72
{"context":"diann-2.2.0","tag":"ghcr.io/${{ github.repository_owner }}/diann:2.2.0","sif":"diann-sif:2.2.0","extra_tags":"ghcr.io/${{ github.repository_owner }}/diann:latest","chg":"CHG_220"},
{"context":"diann-2.1.0","tag":"ghcr.io/${{ github.repository_owner }}/diann:2.1.0","sif":"diann-sif:2.1.0","extra_tags":"","chg":"CHG_210"},
{"context":"diann-2.0.2","tag":"ghcr.io/${{ github.repository_owner }}/diann:2.0.2","sif":"diann-sif:2.0.2","extra_tags":"","chg":"CHG_20"},
{"context":"diann-1.9.2","tag":"ghcr.io/${{ github.repository_owner }}/diann:1.9.2","sif":"diann-sif:1.9.2","extra_tags":"","chg":"CHG_192"},
{"context":"diann-1.8.1","tag":"ghcr.io/${{ github.repository_owner }}/diann:1.8.1","sif":"diann-sif:1.8.1","extra_tags":"","chg":"CHG_181"}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can consider reducing it to only necessary information:

Suggested change
{"context":"diann-2.2.0","tag":"ghcr.io/${{ github.repository_owner }}/diann:2.2.0","sif":"diann-sif:2.2.0","extra_tags":"ghcr.io/${{ github.repository_owner }}/diann:latest","chg":"CHG_220"},
{"context":"diann-2.1.0","tag":"ghcr.io/${{ github.repository_owner }}/diann:2.1.0","sif":"diann-sif:2.1.0","extra_tags":"","chg":"CHG_210"},
{"context":"diann-2.0.2","tag":"ghcr.io/${{ github.repository_owner }}/diann:2.0.2","sif":"diann-sif:2.0.2","extra_tags":"","chg":"CHG_20"},
{"context":"diann-1.9.2","tag":"ghcr.io/${{ github.repository_owner }}/diann:1.9.2","sif":"diann-sif:1.9.2","extra_tags":"","chg":"CHG_192"},
{"context":"diann-1.8.1","tag":"ghcr.io/${{ github.repository_owner }}/diann:1.8.1","sif":"diann-sif:1.8.1","extra_tags":"","chg":"CHG_181"}
{"context":"diann-2.2.0","tags":["2.2.0","latest"],"chg":"CHG_220"},
{"context":"diann-2.1.0","tags":["2.1.0"],"chg":"CHG_210"},
{"context":"diann-2.0.2","tags":["2.0.2"],"chg":"CHG_20"},
{"context":"diann-1.9.2","tags":["1.9.2"],"chg":"CHG_192"},
{"context":"diann-1.8.1","tags":["1.8.1"],"chg":"CHG_181"}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

the tag is actually used in the the way it is atm - so the url is part of it...

@enryH
Copy link
Copy Markdown
Collaborator Author

enryH commented Mar 25, 2026

I cannot merge, so you have to @ypriverol

@enryH
Copy link
Copy Markdown
Collaborator Author

enryH commented Mar 25, 2026

but maybe test the openms job?

@ypriverol
Copy link
Copy Markdown
Member

you can

@enryH
Copy link
Copy Markdown
Collaborator Author

enryH commented Mar 25, 2026

Can I activate the run for testing? at the moment it is skipped.

@enryH
Copy link
Copy Markdown
Collaborator Author

enryH commented Mar 26, 2026

I ran a manual trigger and re-uploaded all images. Only the newly added image was not uploaded. maybe it has to be present for the action to update it?

ERROR: failed to build: failed to solve: failed to push ghcr.io/bigbio/diann:2.3.2: denied: installation not allowed to Write organization package
Error: buildx failed with: ERROR: failed to build: failed to solve: failed to push ghcr.io/bigbio/diann:2.3.2: denied: installation not allowed to Write organization package

@enryH
Copy link
Copy Markdown
Collaborator Author

enryH commented Apr 9, 2026

@ypriverol can i merge if the actions pass?

@ypriverol
Copy link
Copy Markdown
Member

@enryH yes; but the actions are not passing no?

@ypriverol ypriverol changed the base branch from main to dev April 9, 2026 15:52
@ypriverol ypriverol merged commit b16e4b1 into bigbio:dev Apr 9, 2026
4 of 10 checks passed
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.

3 participants