refactor: Migrate sshkey handlers to WithTx#473
Merged
chet merged 2 commits intoNVIDIA:mainfrom May 6, 2026
Merged
Conversation
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 (1)
Summary by CodeRabbit
WalkthroughConsolidates transactional work for Create, Update, and Delete SSH key HTTP handlers by moving DB operations, advisory locks, association CRUD, group version/status and status-detail writes into ChangesSSH Key Handlers — Transactional Consolidation
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 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-05-02 06:11:13 UTC | Commit: fe28e16 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/pkg/api/handler/sshkey.go`:
- Around line 954-974: The loop over skgsasToSync currently performs identical
SSHKeyGroup-level updates and status-detail creations once per site-association,
causing duplicate writes; instead, first deduplicate skgsasToSync by
SSHKeyGroupID (e.g., build a set/map of unique skg IDs from skgsasToSync), then
for each unique SSHKeyGroupID call skgDAO.Update(...) to set Status to Syncing
and call sdDAO.CreateFromParams(...) once per group; after those group-level
writes, iterate the original skgsasToSync for per-site workflow/dispatch work
only, leaving skgDAO.Update and sdDAO.CreateFromParams out of that per-site
loop.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f698d291-504e-446c-8d40-cb2cea9c9920
📒 Files selected for processing (1)
api/pkg/api/handler/sshkey.go
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/pkg/api/handler/sshkey.go`:
- Around line 278-281: The Create handler incorrectly returns an API error
message saying "Failed to delete SSH Key Groups"; change the error text to
reflect the actual operation (e.g., "Failed to update SSH Key Groups") so the
cutil.NewAPIError(...) call and any adjacent log messages
(logger.Error().Err(derr).Msg(...)) consistently describe the update/create
action for SSH Key Groups in sshkey.go.
- Around line 972-975: The API returns a misleading message "Failed to delete
SSH Key Groups" after a DB update error; update the error text passed to
cutil.NewAPIError to accurately describe the operation (e.g., "Failed to update
SSH Key Group status" or "Failed to set SSH Key Group to Syncing") and ensure
other related messages (the logger.Error() call with "error updating SSH Key
Group in DB") remain consistent; locate the failing block using
logger.Error().Err(derr).Msg(...) and cutil.NewAPIError(...) in sshkey.go and
replace the deletion message with the accurate update/syncing message.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3cef4c67-ddab-4254-ac60-398c8a15e0d9
📒 Files selected for processing (1)
api/pkg/api/handler/sshkey.go
thossain-nv
approved these changes
May 5, 2026
Contributor
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Thanks @chet, left one suggestion.
| } | ||
| // Acquire an advisory lock on the SSH Key Group on which there could be contention | ||
| // this lock is released when the transaction commits or rollsback | ||
| if derr = tx.TryAcquireAdvisoryLock(ctx, cdb.GetAdvisoryLockIDFromString(dbskg.ID.String()), nil); derr != nil { |
Contributor
There was a problem hiding this comment.
We tend to separate obtaining error and the conditional into 2 statements. In this case the condition is simple, but the cognitive complexity increase when multiple variables are involved in condition evaluation. So we just always separate them as a general pattern.
Apply the `WithTx` pattern from NVIDIA#462 to the Create/Update/Delete `sshkey` handlers. The post-commit `SyncSSHKeyGroup` workflow triggers stay *outside* the closure (so their log-and-continue error handling doesn't roll back the tx). Keeping these PRs smaller and tightly scoped so they're: - In theory a little easier to read. - More tightly scoped/less "blast radius" per PR. - A little nicer on/for @coderabbitai. 😆 I do I wish the diffs were easier to read, but it is what it is! Signed-off-by: Chet Nichols III <chetn@nvidia.com>
21 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 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.
Description
Apply the
WithTxpattern from #462 to the Create/Update/Deletesshkeyhandlers. The post-commitSyncSSHKeyGroupworkflow triggers stay outside the closure (so their log-and-continue error handling doesn't roll back the tx).Keeping these PRs smaller and tightly scoped so they're:
I do I wish the diffs were easier to read, but it is what it is!
Signed-off-by: Chet Nichols III chetn@nvidia.com
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes