feat: implement issue #747 — [Fleet Monitor] petry-projects/.github — .github/workflows/canary-rollout.yml#748
feat: implement issue #747 — [Fleet Monitor] petry-projects/.github — .github/workflows/canary-rollout.yml#748don-petry wants to merge 3 commits into
Conversation
… .github/workflows/canary-rollout.yml
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe canary rollout helper now captures ChangesCanary workflow handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Code Review
This pull request updates the _run_json function in scripts/canary-rollout.sh to prevent fail-closed errors when a workflow has never been run on a repository. It captures gh command errors in a temporary file and returns an empty array if the workflow is not found, with corresponding tests added in tests/canary_rollout.bats. The review feedback recommends using an EXIT trap to ensure the temporary file is reliably cleaned up, rather than relying on manual deletion at multiple exit points.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #747 by updating the canary rollout orchestration script’s _run_json behavior so tier repositories that don’t have a given workflow are treated as a legitimate “no data” case ([]) rather than a transient outage that triggers retries and ultimately fails the fleet sweep.
Changes:
- Update
_run_jsonto capturegh run liststderr and treat “could not find any workflows named …” as an immediate[]return (no retries, no fail-closed). - Add Bats coverage for the “workflow not present” case, including ensuring it returns immediately without retrying.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/canary_rollout.bats | Adds regression tests for _run_json when a workflow is not present in a tier repo (#747). |
| scripts/canary-rollout.sh | Adjusts _run_json to detect “workflow not present” via captured stderr and return [] instead of retry/fail-closed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/canary-rollout.sh (1)
339-344: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPreserve and log the underlying error message.
By redirecting
gh's stderr to the temporary file$errf, the actual error messages (e.g., "API rate limit exceeded", "HTTP 502", "repository not found") are now swallowed and never shown in the workflow logs for genuine failures. This will make it very difficult to debug why a rollout failed.Please output the contents of
$errfbefore removing it, both on the final failure and during retries, so the root cause remains observable.🛠️ Proposed fix to log the original error
if [ "$attempt" -ge "$attempts" ]; then + cat "$errf" >&2 rm -f "$errf" echo "::error::_run_json: failed to fetch run list for $repo (workflow=$wf) after $attempts attempt(s)" >&2 return 1 fi + cat "$errf" >&2 echo "::warning::_run_json: transient failure fetching run list for $repo (workflow=$wf), attempt $attempt/$attempts — retrying in ${delay}s" >&2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/canary-rollout.sh` around lines 339 - 344, Update the _run_json failure handling to output the contents of $errf before removing it on both the final failure path and each retry path. Preserve the existing error and warning messages while ensuring the captured gh stderr, including the underlying API or repository error, remains visible in workflow logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/canary-rollout.sh`:
- Around line 339-344: Update the _run_json failure handling to output the
contents of $errf before removing it on both the final failure path and each
retry path. Preserve the existing error and warning messages while ensuring the
captured gh stderr, including the underlying API or repository error, remains
visible in workflow logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: deb88725-646a-4f63-acde-0db2a1add666
📒 Files selected for processing (2)
scripts/canary-rollout.shtests/canary_rollout.bats
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #748 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
$errf is function-local; the EXIT trap must capture its value at trap-set time (double-quote expansion) because the variable is out of scope when EXIT fires after the function returns. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Closes #747
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
Bug Fixes
Tests