test slurm integration#121
Closed
dennisklein wants to merge 2 commits into
Closed
Conversation
📝 WalkthroughWalkthroughAdds GitHub Actions CI and Docker image build workflows for automated testing across multiple Fedora/GCC variants and containerized builds. Consolidates legacy Singularity bootstrap definitions into a unified multi-stage Docker Dockerfile. Updates test configurations to parameterize DDS RMS settings. Improves test fixture robustness with safer collection lookups. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 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 (2)
.github/workflows/sindtest.yml (2)
8-10: Add a job timeout.No timeout is specified for this job, which defaults to 360 minutes (6 hours). For a cluster provisioning and verification workflow, this seems excessive and could waste resources if the job hangs.
⏱️ Proposed addition of timeout
sindtest: runs-on: ubuntu-latest + timeout-minutes: 30 steps:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/sindtest.yml around lines 8 - 10, The sindtest job lacks a timeout and can hang for up to the default 360 minutes; add a timeout-minutes entry under the sindtest job (next to runs-on/steps) to cap runtime (e.g., timeout-minutes: 60) so the GitHub Actions job will be automatically cancelled if it exceeds the specified limit.
3-5: Consider adding path filters or branch restrictions.The workflow triggers on all pushes and pull requests without any filters. If SIND cluster testing is only necessary for specific file changes or branches, consider adding path filters or branch restrictions to optimize CI resource usage.
💡 Example with path filters
on: push: + paths-ignore: + - '**.md' + - 'docs/**' pull_request: + paths-ignore: + - '**.md' + - 'docs/**'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/sindtest.yml around lines 3 - 5, The workflow currently triggers on all pushes and pull requests via the on: push: and on: pull_request: entries; restrict these by adding appropriate path filters (paths or paths-ignore) under push and pull_request or branch restrictions (branches/branches-ignore) to limit runs to relevant files or target branches; update the existing on: push and on: pull_request blocks (the workflow trigger definitions) to include the desired paths or branches so the SIND cluster tests run only when specified files change or on selected branches.
🤖 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/sindtest.yml:
- Around line 13-20: The workflow references a non-existent action
"GSI-HPC/sind-action@v1" which will break CI; open
.github/workflows/sindtest.yml, locate the uses: "GSI-HPC/sind-action@v1" line
and either correct it to the actual repository/action name (or tag) that
provides the clusters input or replace it with a valid action (or a local action
path ./action) that supports the same "clusters" input; ensure the chosen action
exists in GitHub (correct owner/repo and version) and update the uses string
accordingly so the workflow can resolve and run successfully.
---
Nitpick comments:
In @.github/workflows/sindtest.yml:
- Around line 8-10: The sindtest job lacks a timeout and can hang for up to the
default 360 minutes; add a timeout-minutes entry under the sindtest job (next to
runs-on/steps) to cap runtime (e.g., timeout-minutes: 60) so the GitHub Actions
job will be automatically cancelled if it exceeds the specified limit.
- Around line 3-5: The workflow currently triggers on all pushes and pull
requests via the on: push: and on: pull_request: entries; restrict these by
adding appropriate path filters (paths or paths-ignore) under push and
pull_request or branch restrictions (branches/branches-ignore) to limit runs to
relevant files or target branches; update the existing on: push and on:
pull_request blocks (the workflow trigger definitions) to include the desired
paths or branches so the SIND cluster tests run only when specified files change
or on selected branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3e382dd-4a5c-4bd7-b69a-5a47e2137827
📒 Files selected for processing (1)
.github/workflows/sindtest.yml
- Add CI workflow for build/test matrix (fedora 39-42, boost 1.90, sanitizers) - Add Docker Images workflow for building/pushing CI container images to GHCR - Add parameterized multi-stage Dockerfile.fedora replacing per-distro Singularity defs - Remove obsolete Singularity definition files (centos 8stream, fedora 36/37/38) - Install boost from dnf by default, build from source only for boost 1.90 variant - Skip image push on pull requests - Upload build/test logs as artifacts on failure - Guard mRuntimeCollectionIndex lookup in test fixture to prevent out-of-range access
- Add sind-node stage to Dockerfile.fedora (systemd, sshd, munge, slurm) - Add fedora42-sind image to docker-images workflow - Add slurm-integration job to CI using sind-action - Make DDS RMS configurable via DDS_RMS CMake variable (defaults to localhost) - Replace hardcoded localhost resource strings in test configs with @DDS_RMS@
dennisklein
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
91-98: Consider quoting the DDS_RMS value to prevent shell interpolation issues.The embedded XML in
-DDDS_RMS="..."contains angle brackets. While this should work as written with the surrounding quotes, ensure the value is properly escaped if it contains special characters in future modifications.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 91 - 98, The DDS_RMS argument containing embedded XML can be misinterpreted by the shell; update the "Build and test" step to pass -DDDS_RMS safely by quoting/escaping the value (for example wrap the entire CMake option in single quotes and keep the XML in double quotes, or use a YAML multiline string or an env var to hold the XML) so the XML angle brackets aren't interpreted by the shell; apply this change where -DDDS_RMS is set in the sind exec odc command to ensure the value is passed unchanged to ctest.
🤖 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 91-98: The DDS_RMS argument containing embedded XML can be
misinterpreted by the shell; update the "Build and test" step to pass -DDDS_RMS
safely by quoting/escaping the value (for example wrap the entire CMake option
in single quotes and keep the XML in double quotes, or use a YAML multiline
string or an env var to hold the XML) so the XML angle brackets aren't
interpreted by the shell; apply this change where -DDDS_RMS is set in the sind
exec odc command to ensure the value is passed unchanged to ctest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6a1c629-9198-4fd7-a74a-933cd2f930b9
📒 Files selected for processing (15)
.github/workflows/ci.yml.github/workflows/docker-images.ymlODCTest.cmaketests/CMakeLists.txttests/cmd_set_1.cfg.intests/cmd_set_1_run.cfg.intests/cmd_set_2.cfg.intests/cmd_set_3_with_script.cfg.intests/odc-fixtures.htests/slurm.cfgutils/ci/Dockerfile.fedorautils/ci/centos.8stream.alice.defutils/ci/fedora36_dds3.7.22.defutils/ci/fedora37_dds3.7.22.defutils/ci/fedora38_dds3.12.def
💤 Files with no reviewable changes (4)
- utils/ci/fedora37_dds3.7.22.def
- utils/ci/fedora36_dds3.7.22.def
- utils/ci/centos.8stream.alice.def
- utils/ci/fedora38_dds3.12.def
✅ Files skipped from review due to trivial changes (4)
- tests/slurm.cfg
- tests/cmd_set_2.cfg.in
- tests/cmd_set_3_with_script.cfg.in
- tests/cmd_set_1.cfg.in
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
2 participants
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.
test