refactor: Migrate networksecuritygroup handler to WithTx#478
Open
chet wants to merge 1 commit intoNVIDIA:mainfrom
Open
refactor: Migrate networksecuritygroup handler to WithTx#478chet wants to merge 1 commit intoNVIDIA:mainfrom
chet wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Applies `WithTx` (and `WithTxResult`!) from NVIDIA#462 to the `Create`/`Update`/`Delete` NSG handlers. Implements our "`timeoutResp` pattern" (which is something we had introduced in NVIDIA#472, and then @coderabbitai said we should be consistent by doing it everywhere). TLDR is the existing code calls `common.TerminateWorkflowOnTimeOut` on timeout, but we want to defer that until after the transaction is unwound + DB connection back (because we don't want it to block waiting on the network). The adjustment (which we've done before, but figured I'd call it out more explicitly here) is effectively: ``` var timeoutResp func() error err = cdb.WithTx(ctx, ..., func(tx *cdb.Tx) error { ... if /* workflow timeout detected */ { // capture the terminate work, but DON'T do it yet timeoutResp = func() error { return common.TerminateWorkflowOnTimeOut(...) } return cutil.NewAPIError(...) // forces rollback } ... }) // rollback has now completed, now we do potentially blocking network work if timeoutResp != nil { return timeoutResp() } ``` Also addressed some @coderabbitai feedback around log messages in advance. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Summary by CodeRabbitRelease NotesNo user-facing changes – This release includes internal code refactoring and optimization of transaction handling for improved reliability and maintainability. End-users will not notice any visible differences in functionality or behavior. WalkthroughManual database transaction handling in the NetworkSecurityGroup Create, Delete, and Update handler methods is replaced with transaction helper functions ( ChangesNetworkSecurityGroup Transaction Management Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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-04 16:58:16 UTC | Commit: 064594a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/pkg/api/handler/networksecuritygroup.go (1)
937-940: 💤 Low valueInconsistent error handling for status detail creation.
In the Create handler (lines 253-256), a failed
sdDAO.CreateFromParamscall returns an error and aborts the transaction. Here in Delete, the error is logged but silently ignored, allowing the deletion to proceed. If this is intentional—treating status detail as non-critical for deletes—consider adding a brief comment to document this design decision; otherwise, align with the Create handler's behavior.💡 Suggested documentation if intentional
// Create status detail + // NOTE: Status detail creation is non-critical for deletes; log and continue. if _, derr := sdDAO.CreateFromParams(ctx, tx, nsg.ID, *cdb.GetStrPtr(cdbm.NetworkSecurityGroupStatusDeleting), cdb.GetStrPtr("received request for deletion, pending processing")); derr != nil { logger.Error().Err(derr).Msg("error creating Status Detail DB entry") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/networksecuritygroup.go` around lines 937 - 940, The Delete handler currently calls sdDAO.CreateFromParams and only logs errors while Create handler treats that error as fatal and aborts the transaction; make them consistent by either (A) propagating the error from sdDAO.CreateFromParams in the Delete handler (roll back/abort the current transaction and return the error exactly like the Create handler does), or (B) if the status-detail write is intentionally non-critical on delete, add a concise comment above the sdDAO.CreateFromParams call explaining this design decision so future readers know the difference; reference sdDAO.CreateFromParams, the Delete handler in networksecuritygroup.go, and the Create handler behavior when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/pkg/api/handler/networksecuritygroup.go`:
- Around line 937-940: The Delete handler currently calls sdDAO.CreateFromParams
and only logs errors while Create handler treats that error as fatal and aborts
the transaction; make them consistent by either (A) propagating the error from
sdDAO.CreateFromParams in the Delete handler (roll back/abort the current
transaction and return the error exactly like the Create handler does), or (B)
if the status-detail write is intentionally non-critical on delete, add a
concise comment above the sdDAO.CreateFromParams call explaining this design
decision so future readers know the difference; reference
sdDAO.CreateFromParams, the Delete handler in networksecuritygroup.go, and the
Create handler behavior when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 32121a4c-e1eb-42a6-8182-d78927e24bf4
📒 Files selected for processing (1)
api/pkg/api/handler/networksecuritygroup.go
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
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
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
Applies
WithTx(andWithTxResult!) from #462 to theCreate/Update/DeleteNSG handlers.Implements our "
timeoutResppattern" (which is something we had introduced in #472, and then @coderabbitai said we should be consistent by doing it everywhere). TLDR is the existing code callscommon.TerminateWorkflowOnTimeOuton timeout, but we want to defer that until after the transaction is unwound + DB connection back (because we don't want it to block waiting on the network).The adjustment (which we've done before, but figured I'd call it out more explicitly here) is effectively:
Also addressed some @coderabbitai feedback around log messages in advance.
Signed-off-by: Chet Nichols III chetn@nvidia.com
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes