This repository was archived by the owner on Jun 2, 2026. It is now read-only.
fix: Log the local serr in workflow status-update fallbacks#595
Open
chet wants to merge 1 commit into
Open
Conversation
Cleans up a recurring "wrong variable logged"... pattern? ...across four workflow activity files. In each spot, an inner `if serr != nil { ... Err(err) }` was logging the outer-scope `err` (instead of the local `serr` returned by the status-update helper) -- so when a status write failed, the log surfaced whatever stale value happened to be in `err` (often nil), masking the real cause.
Doing this because CodeRabbit flagged a similar finding in NVIDIA#594, and since I've been trying to keep everything boilerplate, I was like hrmmm.... so I went back through prior PRs to do a sweep and make sure this was addressed in other call sites that already went in.
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
Summary by CodeRabbit
WalkthroughThis pull request corrects error logging across four activity modules by reporting the actual error values returned by status update operations instead of unrelated outer-scoped error variables. All changes maintain existing control flow and return behavior while improving error diagnostics. ChangesError Logging Fixes in Activity Status Update Handlers
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-06-01 19:58:35 UTC | Commit: 67ad5e6 |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Merged
10 tasks
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
Cleans up a recurring "wrong variable logged"... mmmm pattern? ...across four workflow activity files. In each spot, an inner
if serr != nil { ... Err(err) }was logging the outer-scopeerr(instead of the localserrreturned by the status-update helper) -- so when a status write failed, the log surfaced whatever stale value happened to be inerr(often nil), masking the actual error.Doing this because CodeRabbit flagged a similar finding in #594, and since I've been trying to keep everything boilerplate, I was like hrmmm.... so I went back through prior PRs to do a sweep and make sure this was addressed in other call sites that already went in.
Signed-off-by: Chet Nichols III chetn@nvidia.com
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes