This repository was archived by the owner on Sep 9, 2026. It is now read-only.
NO-ISSUE: Add Run method to TxManager and Tx interfaces - #651
Merged
Merged
Conversation
|
@jhernand: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds reflective Run APIs to Tx/TxManager and migrates tests/mocks to use tm.Run for transactional work. Also adds a runtime proto-file validation tool and its tests. Risk: Medium — review signature validation, panic-to-error conversion, and error-joining semantics. ChangesTransaction Run Abstraction and Test Refactoring
Proto validation tool
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TxManager
participant managedTx
participant runTxTask
participant Task
Caller->>TxManager: Run(ctx, task, args...)
TxManager->>managedTx: Begin()
TxManager->>runTxTask: validate & invoke Task (ctx/Tx + args)
runTxTask->>Task: call via reflect
Task-->>runTxTask: return (optional error)
runTxTask-->>TxManager: taskErr (or panic->error)
alt taskErr != nil
TxManager->>managedTx: ReportError(taskErr)
end
TxManager->>managedTx: End()
TxManager-->>Caller: return taskErr or End error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/database/dao/generic_dao_events_test.go (1)
78-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor reliability risk:
tm.Runerrors are being swallowed, which can mask transactional test failures.Severity: Major. Impact: failed DAO operations/panics inside
tm.Runcan be lost, letting tests proceed with invalid state (false positives/ambiguous failures).
Examples: Line 78 (result not asserted), Lines 104/148/194/391 (callbacks don’t returnerror, assign outererr, thenerr = tm.Run(...)overwrites it), Lines 455/473/482 (step errors overwritten before assertion).🔧 Suggested pattern fix
-err = tm.Run(ctx, func(ctx context.Context) { - _, err = generic.Create(). +err = tm.Run(ctx, func(ctx context.Context) error { + _, err := generic.Create(). SetObject(&privatev1.Cluster{ Metadata: privatev1.Metadata_builder{ Tenant: "my-tenant", }.Build(), }). Do(ctx) -}) + return err +}) Expect(err).ToNot(HaveOccurred())err = tm.Run(ctx, func(ctx context.Context) { _, err = tenantsDao.Create(). SetObject(&privatev1.Organization{ ... }). Do(ctx) Expect(err).ToNot(HaveOccurred()) }) +Expect(err).ToNot(HaveOccurred())As per coding guidelines:
**/*.go: Go security (prodsec-skills): Never ignore error returns.Also applies to: 104-112, 133-145, 148-160, 180-198, 249-261, 311-326, 391-399, 455-487
🤖 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 `@internal/database/dao/generic_dao_events_test.go` around lines 78 - 90, The test is swallowing errors from tm.Run by assigning inner errors to an outer variable or not returning them from the callback (e.g., the block that calls tenantsDao.Create inside tm.Run); change each tm.Run invocation to use the error-returning callback signature (func(ctx context.Context) error), return the inner error from that callback (instead of just assigning to an outer err), then capture and assert the tm.Run error (err = tm.Run(...); Expect(err).ToNot(HaveOccurred())). Update all occurrences referencing tm.Run, tenantsDao.Create, and similar DAO calls so the callback returns errors and tm.Run's returned error is checked (avoid overwriting/losing previous err values).
🤖 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.
Inline comments:
In `@internal/database/database_listener_test.go`:
- Around line 162-167: The helper notify is ignoring the error returned by
tm.Run; update notify to capture tm.Run's error and surface it (e.g., assign err
:= tm.Run(ctx, func(ctx context.Context) { err := notifier.Notify(ctx, payload);
Expect(err).ToNot(HaveOccurred()) }) and then assert
Expect(err).ToNot(HaveOccurred()) or return the error from notify so the caller
can fail the test; ensure you reference the notify helper, tm.Run call, and
notifier.Notify invocation so the transaction-start/end or panic-converted
errors are not silently dropped.
In `@internal/database/database_tx_manager.go`:
- Around line 120-123: The call to tx.End(ctx) stores endErr but the code
returns taskErr and discards endErr; change the finalization logic around
tx.End(ctx) so you never drop endErr: call endErr := tx.End(ctx) then if taskErr
!= nil { if endErr != nil { return fmt.Errorf("task error: %v; tx end error:
%v", taskErr, endErr) } return taskErr } else if endErr != nil { return endErr }
— reference tx.End(ctx), endErr and taskErr and ensure you import/use
appropriate error-wrapping (fmt.Errorf or errors.Join) per project Go version.
---
Outside diff comments:
In `@internal/database/dao/generic_dao_events_test.go`:
- Around line 78-90: The test is swallowing errors from tm.Run by assigning
inner errors to an outer variable or not returning them from the callback (e.g.,
the block that calls tenantsDao.Create inside tm.Run); change each tm.Run
invocation to use the error-returning callback signature (func(ctx
context.Context) error), return the inner error from that callback (instead of
just assigning to an outer err), then capture and assert the tm.Run error (err =
tm.Run(...); Expect(err).ToNot(HaveOccurred())). Update all occurrences
referencing tm.Run, tenantsDao.Create, and similar DAO calls so the callback
returns errors and tm.Run's returned error is checked (avoid overwriting/losing
previous err values).
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 264ca8df-89c4-4dc6-9190-0cd8657ecf17
📒 Files selected for processing (16)
internal/database/dao/generic_dao_events_test.gointernal/database/dao/generic_dao_lock_test.gointernal/database/database_listener_test.gointernal/database/database_notifier_test.gointernal/database/database_tx.gointernal/database/database_tx_manager.gointernal/database/database_tx_manager_mock.gointernal/database/database_tx_manager_test.gointernal/database/database_tx_mock.gointernal/servers/cluster_templates_server_test.gointernal/servers/clusters_server_test.gointernal/servers/compute_instance_catalog_items_server_test.gointernal/servers/compute_instance_templates_server_test.gointernal/servers/console_server_test.gointernal/servers/public_ip_pools_server_test.gointernal/servers/servers_suite_test.go
💤 Files with no reviewable changes (1)
- internal/servers/compute_instance_templates_server_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/database/dao/generic_dao_events_test.go (1)
113-114:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicate error assertion.
Line 114 duplicates the
Expect(err).ToNot(HaveOccurred())check from line 113. The sameerrvariable (assigned fromgeneric.Create().Do(ctx)on line 105) is being asserted twice.Severity: Minor. Impact: Redundant test code with no functional consequence, but indicates a copy-paste oversight.
🔧 Proposed fix
}) Expect(err).ToNot(HaveOccurred()) - Expect(err).ToNot(HaveOccurred()) Expect(event).ToNot(BeNil())🤖 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 `@internal/database/dao/generic_dao_events_test.go` around lines 113 - 114, The test contains a duplicated assertion Expect(err).ToNot(HaveOccurred()) after calling generic.Create().Do(ctx); remove the redundant duplicate so only a single Expect(err).ToNot(HaveOccurred()) remains following the call that sets err (from generic.Create().Do(ctx)), leaving other assertions intact and run the tests to verify.
♻️ Duplicate comments (1)
internal/database/database_listener_test.go (1)
162-168: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider applying the error-returning closure pattern.
The current implementation uses a void closure with error checking inside (line 165) and outside (line 167). A previous review suggested refactoring to an error-returning closure for cleaner code:
notify := func(ctx context.Context, payload proto.Message) { err := tm.Run(ctx, func(ctx context.Context) error { return notifier.Notify(ctx, payload) }) Expect(err).ToNot(HaveOccurred()) }Severity: Recommended refactor. Impact: The current pattern is functional but less idiomatic than the error-returning pattern used elsewhere in the codebase (e.g.,
generic_dao_events_test.golines 215–231, 273–281).♻️ Proposed refactor
notify := func(ctx context.Context, payload proto.Message) { - err := tm.Run(ctx, func(ctx context.Context) { - err := notifier.Notify(ctx, payload) - Expect(err).ToNot(HaveOccurred()) - }) + err := tm.Run(ctx, func(ctx context.Context) error { + return notifier.Notify(ctx, payload) + }) Expect(err).ToNot(HaveOccurred()) }🤖 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 `@internal/database/database_listener_test.go` around lines 162 - 168, Refactor the notify closure to return the error from the tm.Run call instead of doing nested Expect checks: make notify accept (ctx context.Context, payload proto.Message) and call tm.Run with a closure that returns notifier.Notify(ctx, payload), capture the error from tm.Run and then call Expect(err).ToNot(HaveOccurred()); update the closure signature and the inner tm.Run callback to return error so the outer error handling is singular and idiomatic (references: notify closure, tm.Run, notifier.Notify).
🤖 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.
Inline comments:
In `@internal/database/database_tx_manager.go`:
- Around line 267-279: Validate each entry of args against the expected
parameter type on taskType before building callArgs or invoking taskFunc.Call:
for each index i, get paramType := taskType.In(i+1) (since firstArg is already
handled), then if args[i] is nil only allow it when paramType is a nil-able kind
(Ptr, Slice, Map, Func, Chan, Interface) and otherwise return an error from Run;
if args[i] is non-nil obtain rv := reflect.ValueOf(args[i]) and ensure
rv.Type().AssignableTo(paramType) (or explicitly convert only if you intend to
allow conversions) and return an error if not assignable; only after all args
validate, construct callArgs (using reflect.Zero(paramType) for allowed nils)
and call taskFunc.Call(callArgs). This prevents silent coercion and reflect.Call
panics (references: taskType, args, callArgs, taskFunc, Run).
---
Outside diff comments:
In `@internal/database/dao/generic_dao_events_test.go`:
- Around line 113-114: The test contains a duplicated assertion
Expect(err).ToNot(HaveOccurred()) after calling generic.Create().Do(ctx); remove
the redundant duplicate so only a single Expect(err).ToNot(HaveOccurred())
remains following the call that sets err (from generic.Create().Do(ctx)),
leaving other assertions intact and run the tests to verify.
---
Duplicate comments:
In `@internal/database/database_listener_test.go`:
- Around line 162-168: Refactor the notify closure to return the error from the
tm.Run call instead of doing nested Expect checks: make notify accept (ctx
context.Context, payload proto.Message) and call tm.Run with a closure that
returns notifier.Notify(ctx, payload), capture the error from tm.Run and then
call Expect(err).ToNot(HaveOccurred()); update the closure signature and the
inner tm.Run callback to return error so the outer error handling is singular
and idiomatic (references: notify closure, tm.Run, notifier.Notify).
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5944cf81-5718-42db-b8de-7ebba512105f
📒 Files selected for processing (16)
internal/database/dao/generic_dao_events_test.gointernal/database/dao/generic_dao_lock_test.gointernal/database/database_listener_test.gointernal/database/database_notifier_test.gointernal/database/database_tx.gointernal/database/database_tx_manager.gointernal/database/database_tx_manager_mock.gointernal/database/database_tx_manager_test.gointernal/database/database_tx_mock.gointernal/servers/cluster_templates_server_test.gointernal/servers/clusters_server_test.gointernal/servers/compute_instance_catalog_items_server_test.gointernal/servers/compute_instance_templates_server_test.gointernal/servers/console_server_test.gointernal/servers/public_ip_pools_server_test.gointernal/servers/servers_suite_test.go
💤 Files with no reviewable changes (1)
- internal/servers/compute_instance_templates_server_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@internal/proto/proto_tool_test.go`:
- Around line 21-59: Add negative tests for proto.Tool to cover Build() and
Check() error paths: create new It blocks that call NewTool().SetLogger(logger)
/ NewTool() without SetLogger, and NewTool().AddPackage(...) with deliberately
invalid/missing fixtures (e.g., descriptors lacking expected message, missing id
or metadata) then call Build() and assert Build() returns an error for missing
required builder inputs, and call tool.Check() on a built tool with bad
descriptors to assert specific validation errors are returned; reference
NewTool(), SetLogger, AddPackage, AddExclude, Build, and Tool.Check to locate
where to construct failing cases and assert exact error messages.
- Line 46: The test suite is failing because
proto/private/osac/private/v1/condition_status_type.proto defines only an enum
(ConditionStatus) but the checker in internal/proto/proto_tool.go expects a
message with id and metadata for *_type.proto files; re-enable the private
exclusion by uncommenting the
AddExclude("osac/private/v1/condition_status_type.proto") call in
internal/proto/proto_tool_test.go (near the AddExclude list) or alternatively
modify internal/proto/proto_tool.go to detect enum-only files and skip the
message/id/metadata validation for files like condition_status_type.proto;
reference the AddExclude(...) call, proto_tool_test.go, proto_tool.go, and
condition_status_type.proto/ConditionStatus when making the change.
In `@internal/proto/proto_tool.go`:
- Around line 91-148: The Tool struct's logger (set during Build) is never used
in Check or checkFile; either remove the logger from the builder/Tool API
(delete Tool.logger and update Build) or wire it into the validation flow by
using Tool.logger in Check and checkFile to emit debug/info logs when files are
skipped (package/excludes), when a file matches the "_type.proto" suffix, when
expectedMessageName is computed, when findMessage returns nil, and when
id/metadata fields are missing; reference Tool.logger, Tool.Build, Tool.Check,
Tool.checkFile, expectedMessageName, and findMessage to locate where to add or
remove the logger usage.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 946a308c-f91d-40e0-b5c8-563bd92d6cd6
📒 Files selected for processing (19)
internal/database/dao/generic_dao_events_test.gointernal/database/dao/generic_dao_lock_test.gointernal/database/database_listener_test.gointernal/database/database_notifier_test.gointernal/database/database_tx.gointernal/database/database_tx_manager.gointernal/database/database_tx_manager_mock.gointernal/database/database_tx_manager_test.gointernal/database/database_tx_mock.gointernal/proto/proto_suite_test.gointernal/proto/proto_tool.gointernal/proto/proto_tool_test.gointernal/servers/cluster_templates_server_test.gointernal/servers/clusters_server_test.gointernal/servers/compute_instance_catalog_items_server_test.gointernal/servers/compute_instance_templates_server_test.gointernal/servers/console_server_test.gointernal/servers/public_ip_pools_server_test.gointernal/servers/servers_suite_test.go
💤 Files with no reviewable changes (1)
- internal/servers/compute_instance_templates_server_test.go
Add a reflection-based `Run` method that executes a task function within a database transaction, automatically handling commit, rollback, and panic recovery. The task function's first parameter must be either `context.Context` (in which case the transaction is stored in the context) or `database.Tx` (passed directly). Additional parameters of any type can be supplied via variadic args. If the last return value implements `error`, it determines whether to commit or rollback. `TxManager.Run` creates a new transaction, runs the task, and ends the transaction. `Tx.Run` executes the task within an existing transaction, reporting errors but leaving lifecycle management to the caller. Assisted-by: Cursor Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
|
@jhernand: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
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.
Summary
Runmethod to both theTxManagerandTxinterfaces that executesa task function within a database transaction, automatically handling commit, rollback, and panic
recovery.
context.Contextordatabase.Tx, withadditional parameters of any type passed via variadic args. If the last return value implements
error, it determines whether to commit or rollback.TxManager.Runcreates a new transaction, runs the task, and ends the transaction.Tx.Runexecutes the task within an existing transaction, reporting errors but leaving lifecyclemanagement to the caller.
Test plan
func(context.Context),func(context.Context) error,func(Tx),func(Tx) error)ReportErrortriggering rollbackTx.Run(commit, rollback on error, rollback on panic, extra args,context-based task)
internal/databasetest suite passes (83 tests)Summary by CodeRabbit
Note: No user-facing behavior changed.