Skip to content

fix(rest-api): Log the local serr in workflow status-update fallbacks#2120

Merged
chet merged 1 commit into
NVIDIA:mainfrom
chet:rest-tweak-existing-proto-err-vars
Jun 2, 2026
Merged

fix(rest-api): Log the local serr in workflow status-update fallbacks#2120
chet merged 1 commit into
NVIDIA:mainfrom
chet:rest-tweak-existing-proto-err-vars

Conversation

@chet

@chet chet commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Description

This pulls over NVIDIA/infra-controller-rest#595 to our new repo.

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 #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

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Related Issues (Optional)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Additional Notes

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>
@chet chet requested a review from a team as a code owner June 2, 2026 18:57
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-06-02 19:00:13 UTC | Commit: 12e7917

@chet

chet commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai There is a .coderabbit.yaml in rest-api/.coderabbit.yaml in this repo that you can use, with a rest-api/AGENTS.md with general guidelines. This PR actually should be good to go based on previous work we've done, but looping you in for funsies anyway to say hi.

@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== rest-api/.coderabbit.yaml ==="
cat rest-api/.coderabbit.yaml 2>/dev/null || echo "File not found"

echo ""
echo "=== rest-api/AGENTS.md ==="
cat rest-api/AGENTS.md 2>/dev/null || echo "File not found"

echo ""
echo "=== PR diff summary ==="
gh pr view 2120 --repo NVIDIA/infra-controller --json title,body,files 2>/dev/null | jq '{title: .title, files: [.files[].path]}'

echo ""
echo "=== PR diff ==="
gh pr diff 2120 --repo NVIDIA/infra-controller 2>/dev/null | head -200

Length of output: 36379



@chet Hey, hi! 👋 Got the config — rest-api/.coderabbit.yaml and AGENTS.md are both loaded in. Great set of conventions in there.

Took a look through the diff: all seven Err(err)Err(serr) substitutions across the four activity files look correct and consistent. Each if serr != nil block now surfaces the actual status-update error rather than whatever stale value happened to be sitting in the outer err. Clean sweep, nothing left behind.

Ship it. 🐰✨

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

🔍 Container Scan Summary

Service Total Critical High Medium Low Other
nico-flow 66 4 34 18 2 8
nico-nsm 82 2 28 43 9 0
nico-psm 67 4 35 18 2 8
nico-rest-api 100 6 53 30 3 8
nico-rest-cert-manager 65 4 34 18 1 8
nico-rest-db 66 4 34 18 2 8
nico-rest-site-agent 65 4 34 18 1 8
nico-rest-site-manager 65 4 34 18 1 8
nico-rest-workflow 67 4 35 18 2 8
TOTAL 643 36 321 199 23 64

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.

@chet chet merged commit df435a8 into NVIDIA:main Jun 2, 2026
88 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants