Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe script Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
|
The following will be added to the changelog [0.6.1] - 2026-03-19Features
|
veeso
force-pushed
the
feat/504-get-peer_end_count-after-running-nomad-scenarios
branch
4 times, most recently
from
d5e0380 to
6de2fbe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
nomad/scripts/ci_allocs.sh (2)
77-77: Nitpick: Movelocaldeclaration outside the loop.Declaring
localinside the loop re-declares on each iteration. Moving it before the loop is cleaner.✨ Suggested improvement
declare -A peer_end_count_by_run + local this_alloc_peer_end_count while IFS=',' read -r _job_name _scenario_name alloc_id _run_id _started_at _duration _peer_count _behaviours; do - local this_alloc_peer_end_count this_alloc_peer_end_count=$(fetch_peer_end_count "$alloc_id" "$_peer_count") peer_end_count_by_run["$_run_id"]="$(( ${peer_end_count_by_run["$_run_id"]:-0} + this_alloc_peer_end_count ))" done < "$allocs_csv_file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nomad/scripts/ci_allocs.sh` at line 77, Move the variable declaration for this_alloc_peer_end_count out of the loop to avoid re-declaring it on each iteration; declare it once as local before the loop that uses it (reference the variable name this_alloc_peer_end_count and the enclosing loop in nomad/scripts/ci_allocs.sh) and keep assignments/updates inside the loop.
150-162: Pipeline error handling may behave unexpectedly withset -eo pipefail.With
set -eo pipefailat the top of the script, the|| result=""at the end of line 154 should catch failures, but the behavior can be subtle. Ifnomadsucceeds butjqfails (e.g., malformed JSON),pipefailwould cause the pipeline to return non-zero, and|| result=""would then execute—this is likely the intended behavior.However, there's a subtle case: if the file exists but is empty,
jq --slurp 'last'returnsnull, and// 0yields0. This silently reports zero peers completed instead of triggering the fallback. Consider whether this is acceptable or if you want to treat0as a warning condition too.💡 Optional: Add explicit check for zero result
function fetch_peer_end_count() { local alloc_id="$1" local fallback_peer_count="$2" local result result=$(nomad alloc fs "$alloc_id" alloc/run_summary.jsonl 2>/dev/null | jq --slurp 'last | .peer_end_count // 0') || result="" - if [[ -z "$result" || "$result" == "null" ]]; then + if [[ -z "$result" || "$result" == "null" || "$result" == "0" ]]; then echo "Warning: could not fetch peer_end_count for alloc $alloc_id, falling back to configured peer_count ($fallback_peer_count)" >&2 echo "$fallback_peer_count" return fi echo "Fetched peer_end_count for alloc $alloc_id: $result" >&2 echo "$result" }Alternatively, if
0is a valid value (e.g., all agents failed), the current behavior is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nomad/scripts/ci_allocs.sh` around lines 150 - 162, In fetch_peer_end_count, handle the empty-file vs legitimate-zero ambiguity by first capturing nomad output into a raw variable (e.g., raw_output) and checking if raw_output is empty before running jq; if raw_output is empty or nomad/jq exit non-zero, log the warning and echo the fallback_peer_count, otherwise run jq on raw_output and if jq returns null/empty treat as fallback too, finally echo the numeric result; update references inside fetch_peer_end_count (alloc_id, fallback_peer_count, result) accordingly so the pipeline failure and empty-file cases both fall back instead of silently returning 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nomad/scripts/ci_allocs.sh`:
- Line 77: Move the variable declaration for this_alloc_peer_end_count out of
the loop to avoid re-declaring it on each iteration; declare it once as local
before the loop that uses it (reference the variable name
this_alloc_peer_end_count and the enclosing loop in nomad/scripts/ci_allocs.sh)
and keep assignments/updates inside the loop.
- Around line 150-162: In fetch_peer_end_count, handle the empty-file vs
legitimate-zero ambiguity by first capturing nomad output into a raw variable
(e.g., raw_output) and checking if raw_output is empty before running jq; if
raw_output is empty or nomad/jq exit non-zero, log the warning and echo the
fallback_peer_count, otherwise run jq on raw_output and if jq returns null/empty
treat as fallback too, finally echo the numeric result; update references inside
fetch_peer_end_count (alloc_id, fallback_peer_count, result) accordingly so the
pipeline failure and empty-file cases both fall back instead of silently
returning 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53c51d9a-edf8-42e1-aa28-fa42ecbeb08e
📒 Files selected for processing (1)
nomad/scripts/ci_allocs.sh
…n summaries inside of allocations by their run_id closes #504
veeso
force-pushed
the
feat/504-get-peer_end_count-after-running-nomad-scenarios
branch
from
6de2fbe to
f71d584
Compare
|
✔️ f71d584 - Conventional commits check succeeded. |
veeso
deleted the
feat/504-get-peer_end_count-after-running-nomad-scenarios
branch
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.
Summary
When running scenarios via Nomad, the
peer_end_countin the generatedrun_summary.jsonlwas always set to the configuredpeer_count, ignoring agents that may have dropped during the run. This PR fixes that by fetching each allocation'srun_summary.jsonlvia the Nomad API, reading the actualpeer_end_count, and summing them perrun_id.Changes
fetch_peer_end_countfunction that readspeer_end_countfrom an allocation'srun_summary.jsonlvianomad alloc fs, with a fallback to the configuredpeer_countif the file is unavailable.peer_end_count_by_runmap keyed byrun_id.peer_end_countinstead of hardcoding it topeer_countin the jq template.closes #504
TODO:
Note that all commits in a PR must follow Conventional Commits before it can be merged, as these are used to generate the changelog
Summary by CodeRabbit