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:
📝 WalkthroughWalkthroughThis PR modernizes the CI infrastructure by adding GitHub Actions workflows for automated testing and Docker image building, updating container definitions from older CentOS/Fedora versions to Fedora 39–42 variants, and removing obsolete Singularity definitions. The new setup includes matrix-based testing across multiple Fedora versions and an automated Docker image registry push. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🧹 Nitpick comments (5)
utils/ci/Dockerfile.fedora42-boost190 (2)
26-26: Unnecessarydnf removecommand.
boost-develis not installed in lines 3-24 (unlike other Fedora Dockerfiles), so this remove command is a no-op. It can be safely removed to avoid confusion and reduce build time.♻️ Proposed fix
-RUN dnf -y remove 'boost-devel' && dnf -y clean all - RUN wget https://archives.boost.io/release/1.90.0/source/boost_1_90_0.tar.gz && \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/ci/Dockerfile.fedora42-boost190` at line 26, Remove the unnecessary package removal step: delete the RUN instruction that executes "dnf -y remove 'boost-devel' && dnf -y clean all" in the Fedora42 Boost Dockerfile so the image build doesn't run a no-op removal; keep any subsequent cache cleanup or other commands intact if needed.
28-33: Consider adding SHA256 checksum verification for download integrity.The Boost 1.90.0 URL is correct, but adding checksum validation would improve security and detect potential corruption during download.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/ci/Dockerfile.fedora42-boost190` around lines 28 - 33, Add SHA256 checksum verification for the Boost tarball in the same RUN sequence that downloads boost_1_90_0.tar.gz: obtain the known SHA256 for boost_1_90_0.tar.gz and after wget compute and check the checksum (e.g., via sha256sum -c or echo "<sha256> boost_1_90_0.tar.gz" | sha256sum -c -) and abort the build if verification fails, then proceed to tar, build (./bootstrap.sh and ./b2 install) and cleanup only when the checksum is valid; update the existing RUN command that references boost_1_90_0.tar.gz accordingly..github/workflows/ci.yml (2)
47-50: Consider quoting thematrix.extraexpansion to handle empty values safely.When
matrix.extrais not defined for a matrix entry, the unquoted expansion may cause issues depending on shell behavior. Explicitly defaulting to an empty string ensures predictable behavior.💡 Proposed fix
- name: Build and test env: LABEL: ${{ matrix.label }} - run: ctest -S ODCTest.cmake -VV --output-on-failure ${{ matrix.extra }} + run: ctest -S ODCTest.cmake -VV --output-on-failure ${{ matrix.extra || '' }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 47 - 50, In the "Build and test" job update the run command to safely expand matrix.extra by quoting it and defaulting to an empty string; specifically change the ctest invocation that uses ${{ matrix.extra }} so it becomes a quoted expansion with a fallback (e.g. "${{ matrix.extra || '' }}" or equivalent) to avoid errors when matrix.extra is undefined while keeping the rest of the step (env: LABEL, name: Build and test) unchanged.
37-41: Useenv.IMAGE_PREFIXinstead of hardcoding the image path.Line 38 hardcodes
'ghcr.io/fairrootgroup/odc-ci'whileIMAGE_PREFIXis defined in the env section (line 10) with the same value. Using the env variable improves maintainability.♻️ Proposed fix
container: - image: ${{ format('{0}:{1}', 'ghcr.io/fairrootgroup/odc-ci', matrix.fedora) }} + image: ${{ env.IMAGE_PREFIX }}:${{ matrix.fedora }} credentials: username: ${{ github.actor }} password: ${{ secrets.GITHUB_TOKEN }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 37 - 41, Replace the hardcoded image prefix in the container.image expression with the env variable IMAGE_PREFIX: update the format call currently using the literal 'ghcr.io/fairrootgroup/odc-ci' to use ${{ env.IMAGE_PREFIX }} so container.image is built from IMAGE_PREFIX and matrix.fedora (i.e., keep the format('{0}:{1}', ..., matrix.fedora) but pass ${{ env.IMAGE_PREFIX }} instead of the literal); this fixes the use of IMAGE_PREFIX declared in env and avoids duplicating the registry string.utils/ci/Dockerfile.fedora41 (1)
27-30: Consider pinning FairCMakeModules to a specific version tag.Unlike the other dependencies (FairLogger v2.3.1, FairMQ v1.10.1, DDS 3.16), FairCMakeModules is cloned from the default branch without a version pin. This could lead to build inconsistencies if the upstream default branch changes.
💡 Proposed fix
-RUN git clone https://github.com/FairRootGroup/FairCMakeModules && \ +RUN git clone -b <version_tag> https://github.com/FairRootGroup/FairCMakeModules && \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/ci/Dockerfile.fedora41` around lines 27 - 30, The Dockerfile clones FairCMakeModules without a pinned version; modify the RUN step that invokes "git clone https://github.com/FairRootGroup/FairCMakeModules" so it checks out a specific tag or commit (e.g., use "git clone --branch <TAG> --depth 1" or clone then "git checkout <COMMIT/TAG>") before running "cmake -GNinja -S FairCMakeModules -B FairCMakeModules_build" and the subsequent build/install steps, ensuring FairCMakeModules is fixed to a reproducible version.
🤖 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/docker-images.yml:
- Around line 46-47: Update the GitHub Actions step that currently uses
docker/build-push-action@v5 (the step named "Build and push") to the v6 series
by changing the uses reference to docker/build-push-action@v6; keep the step
name and existing inputs intact but ensure the action version string is updated
to v6 (e.g., docker/build-push-action@v6) so the workflow uses the current
stable release.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 47-50: In the "Build and test" job update the run command to
safely expand matrix.extra by quoting it and defaulting to an empty string;
specifically change the ctest invocation that uses ${{ matrix.extra }} so it
becomes a quoted expansion with a fallback (e.g. "${{ matrix.extra || '' }}" or
equivalent) to avoid errors when matrix.extra is undefined while keeping the
rest of the step (env: LABEL, name: Build and test) unchanged.
- Around line 37-41: Replace the hardcoded image prefix in the container.image
expression with the env variable IMAGE_PREFIX: update the format call currently
using the literal 'ghcr.io/fairrootgroup/odc-ci' to use ${{ env.IMAGE_PREFIX }}
so container.image is built from IMAGE_PREFIX and matrix.fedora (i.e., keep the
format('{0}:{1}', ..., matrix.fedora) but pass ${{ env.IMAGE_PREFIX }} instead
of the literal); this fixes the use of IMAGE_PREFIX declared in env and avoids
duplicating the registry string.
In `@utils/ci/Dockerfile.fedora41`:
- Around line 27-30: The Dockerfile clones FairCMakeModules without a pinned
version; modify the RUN step that invokes "git clone
https://github.com/FairRootGroup/FairCMakeModules" so it checks out a specific
tag or commit (e.g., use "git clone --branch <TAG> --depth 1" or clone then "git
checkout <COMMIT/TAG>") before running "cmake -GNinja -S FairCMakeModules -B
FairCMakeModules_build" and the subsequent build/install steps, ensuring
FairCMakeModules is fixed to a reproducible version.
In `@utils/ci/Dockerfile.fedora42-boost190`:
- Line 26: Remove the unnecessary package removal step: delete the RUN
instruction that executes "dnf -y remove 'boost-devel' && dnf -y clean all" in
the Fedora42 Boost Dockerfile so the image build doesn't run a no-op removal;
keep any subsequent cache cleanup or other commands intact if needed.
- Around line 28-33: Add SHA256 checksum verification for the Boost tarball in
the same RUN sequence that downloads boost_1_90_0.tar.gz: obtain the known
SHA256 for boost_1_90_0.tar.gz and after wget compute and check the checksum
(e.g., via sha256sum -c or echo "<sha256> boost_1_90_0.tar.gz" | sha256sum -c
-) and abort the build if verification fails, then proceed to tar, build
(./bootstrap.sh and ./b2 install) and cleanup only when the checksum is valid;
update the existing RUN command that references boost_1_90_0.tar.gz accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ec2b6e2-1f8b-4871-93dd-86722ae6cdef
📒 Files selected for processing (10)
.github/workflows/ci.yml.github/workflows/docker-images.ymlutils/ci/Dockerfile.fedora39utils/ci/Dockerfile.fedora40utils/ci/Dockerfile.fedora41utils/ci/Dockerfile.fedora42utils/ci/Dockerfile.fedora42-boost190utils/ci/centos.8stream.alice.defutils/ci/fedora36_dds3.7.22.defutils/ci/fedora37_dds3.7.22.def
💤 Files with no reviewable changes (3)
- utils/ci/fedora37_dds3.7.22.def
- utils/ci/centos.8stream.alice.def
- utils/ci/fedora36_dds3.7.22.def
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/docker-images.yml (1)
7-8: Align the path filter with the build context.The workflow watches only
utils/ci/Dockerfile.*, but the build context is the entireutils/cidirectory. If any helper file is added underutils/cilater, changing it will affect the image without triggering a rebuild.Suggested change
paths: - - 'utils/ci/Dockerfile.*' + - 'utils/ci/**'Also applies to: 49-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docker-images.yml around lines 7 - 8, The workflow's path filter currently only matches 'utils/ci/Dockerfile.*' so changes to other files in the build context won't trigger a rebuild; update the workflow's paths filter entries (the YAML key "paths") to include the whole build context (e.g., 'utils/ci/**' or 'utils/ci/**/*') instead of just 'utils/ci/Dockerfile.*' and apply the same change to the other occurrence referenced in the file (the second paths entry around the other build job).
🤖 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/docker-images.yml:
- Around line 3-9: The workflow currently only triggers on push to master so
Dockerfile changes don't build for PRs; update the workflow's trigger block by
adding a pull_request trigger (matching the same branches and paths as the
existing push trigger) so image builds run for PRs before merge, and also update
the other 'on:' occurrence in this same workflow (the second trigger block) to
mirror this change; ensure you keep the existing paths filter
('utils/ci/Dockerfile.*') and workflow_dispatch behavior.
---
Nitpick comments:
In @.github/workflows/docker-images.yml:
- Around line 7-8: The workflow's path filter currently only matches
'utils/ci/Dockerfile.*' so changes to other files in the build context won't
trigger a rebuild; update the workflow's paths filter entries (the YAML key
"paths") to include the whole build context (e.g., 'utils/ci/**' or
'utils/ci/**/*') instead of just 'utils/ci/Dockerfile.*' and apply the same
change to the other occurrence referenced in the file (the second paths entry
around the other build job).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e1b479c-959b-4f25-9535-830036be3c30
📒 Files selected for processing (1)
.github/workflows/docker-images.yml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
37-41: Consider using theIMAGE_PREFIXenv variable for consistency.The
IMAGE_PREFIXenvironment variable is defined on line 10 but not used in the container image specification. Using it would reduce duplication and make future changes easier.Suggested change
container: - image: ${{ format('{0}:{1}', 'ghcr.io/fairrootgroup/odc-ci', matrix.fedora) }} + image: ${{ format('{0}:{1}', env.IMAGE_PREFIX, matrix.fedora) }} credentials: username: ${{ github.actor }} password: ${{ secrets.GITHUB_TOKEN }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 37 - 41, The container image line currently hardcodes the prefix 'ghcr.io/fairrootgroup/odc-ci' instead of using the existing IMAGE_PREFIX env var; update the container.image expression to use IMAGE_PREFIX combined with matrix.fedora (e.g., format('{0}:{1}', env.IMAGE_PREFIX, matrix.fedora) or equivalent) so container.image references IMAGE_PREFIX and matrix.fedora for consistency and to avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 37-41: The container image line currently hardcodes the prefix
'ghcr.io/fairrootgroup/odc-ci' instead of using the existing IMAGE_PREFIX env
var; update the container.image expression to use IMAGE_PREFIX combined with
matrix.fedora (e.g., format('{0}:{1}', env.IMAGE_PREFIX, matrix.fedora) or
equivalent) so container.image references IMAGE_PREFIX and matrix.fedora for
consistency and to avoid duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 876d270a-8761-4978-896a-4235e2e1733a
📒 Files selected for processing (2)
.github/workflows/ci.ymlCMakeLists.txt
There was a problem hiding this comment.
🧹 Nitpick comments (2)
utils/ci/Dockerfile.fedora39 (1)
27-30: Consider pinning FairCMakeModules to a specific tag or commit.Same as in
Dockerfile.fedora40— cloning without a version specifier may lead to non-reproducible builds.💡 Suggested fix
-RUN git clone https://github.com/FairRootGroup/FairCMakeModules && \ +RUN git clone -b <TAG_OR_COMMIT> https://github.com/FairRootGroup/FairCMakeModules && \ cmake -GNinja -S FairCMakeModules -B FairCMakeModules_build -DCMAKE_INSTALL_PREFIX=/usr && \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/ci/Dockerfile.fedora39` around lines 27 - 30, The Dockerfile clones FairCMakeModules without pinning, causing unreproducible builds; modify the RUN sequence that references FairCMakeModules (the git clone / cmake / build / rm commands) to clone a specific tag or commit (e.g., use git clone --branch <TAG> --depth 1 or clone then git checkout <COMMIT>) so the subsequent cmake invocation (FairCMakeModules -> FairCMakeModules_build) always builds a known revision, and remove transient files as before.utils/ci/Dockerfile.fedora40 (1)
27-30: Consider pinning FairCMakeModules to a specific tag or commit.Unlike the other dependencies (FairLogger, FairMQ, DDS), FairCMakeModules is cloned without a version specifier, which may lead to non-reproducible builds if upstream changes.
💡 Suggested fix
-RUN git clone https://github.com/FairRootGroup/FairCMakeModules && \ +RUN git clone -b <TAG_OR_COMMIT> https://github.com/FairRootGroup/FairCMakeModules && \ cmake -GNinja -S FairCMakeModules -B FairCMakeModules_build -DCMAKE_INSTALL_PREFIX=/usr && \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/ci/Dockerfile.fedora40` around lines 27 - 30, The Dockerfile currently clones FairCMakeModules without a version, risking non-reproducible builds; modify the RUN block that clones and builds FairCMakeModules so it checks out a specific tag or commit (e.g., use git clone --branch <TAG> --depth 1 https://github.com/FairRootGroup/FairCMakeModules or clone then git checkout <COMMIT_HASH> before running cmake and the existing build/install steps for FairCMakeModules and FairCMakeModules_build) so the build uses a pinned revision.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@utils/ci/Dockerfile.fedora39`:
- Around line 27-30: The Dockerfile clones FairCMakeModules without pinning,
causing unreproducible builds; modify the RUN sequence that references
FairCMakeModules (the git clone / cmake / build / rm commands) to clone a
specific tag or commit (e.g., use git clone --branch <TAG> --depth 1 or clone
then git checkout <COMMIT>) so the subsequent cmake invocation (FairCMakeModules
-> FairCMakeModules_build) always builds a known revision, and remove transient
files as before.
In `@utils/ci/Dockerfile.fedora40`:
- Around line 27-30: The Dockerfile currently clones FairCMakeModules without a
version, risking non-reproducible builds; modify the RUN block that clones and
builds FairCMakeModules so it checks out a specific tag or commit (e.g., use git
clone --branch <TAG> --depth 1 https://github.com/FairRootGroup/FairCMakeModules
or clone then git checkout <COMMIT_HASH> before running cmake and the existing
build/install steps for FairCMakeModules and FairCMakeModules_build) so the
build uses a pinned revision.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1396b51-4cc2-4c39-8b8a-054014db11e5
📒 Files selected for processing (2)
utils/ci/Dockerfile.fedora39utils/ci/Dockerfile.fedora40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.