NO-ISSUE: Move End method from TxManager to Tx interface

[jhernand]

Summary

• Moves the End method from the TxManager interface to the Tx interface so that
  callers can simply call tx.End(ctx) instead of manager.End(ctx, tx). The
  managedTx implementation already holds a back-reference to its manager, so there
  is no need for the transaction manager to mediate the commit/rollback decision.
• Consolidates the commit/rollback logic that was previously split across
  txManager.End and txManager.end into a single managedTx.End method, removing
  the type-switch dispatch and the cross-manager ownership check.
• Updates all production callers, test callers, interceptor mock expectations, and
  regenerated mocks across 26 files.

Test plan

• go build ./... compiles successfully
• ginkgo run -r internal passes all 62 test suites

Summary by CodeRabbit

• Refactor
  • Internal transaction management refactoring: transaction completion is now handled directly on transaction objects rather than through a separate transaction manager interface. This change affects how transactions are finalized internally with no impact to end-user functionality or features.

[openshift-ci-robot]

@jhernand: This pull request explicitly references no jira issue.

Details

In response to this:

Summary

• Moves the End method from the TxManager interface to the Tx interface so that
  callers can simply call tx.End(ctx) instead of manager.End(ctx, tx). The
  managedTx implementation already holds a back-reference to its manager, so there
  is no need for the transaction manager to mediate the commit/rollback decision.
• Consolidates the commit/rollback logic that was previously split across
  txManager.End and txManager.end into a single managedTx.End method, removing
  the type-switch dispatch and the cross-manager ownership check.
• Updates all production callers, test callers, interceptor mock expectations, and
  regenerated mocks across 26 files.

Test plan

• go build ./... compiles successfully
• ginkgo run -r internal passes all 62 test suites

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.

[openshift-ci]

[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

Details

Needs approval from an approver in each of these files:

• OWNERS [jhernand]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

This PR refactors transaction lifecycle management by moving the End(ctx) responsibility from the TxManager interface onto individual Tx transaction objects. The contract change propagates through the gRPC interceptor, all database tests, and server implementations, replacing ~44 tm.End(ctx, tx) calls with tx.End(ctx).

Changes

Transaction Lifecycle Refactoring

Layer / File(s)	Summary
Transaction API contract and core implementation internal/database/database_tx.go, internal/database/database_tx_manager.go, internal/database/database_tx_mock.go	Tx interface gains End(ctx) error method. TxManager interface loses End(ctx, tx) method entirely. managedTx gains the End(ctx) implementation that commits or rolls back based on errors reported via ReportError, logs the decision, and collects errors on rollback. MockTxManager.End mock is removed; MockTx.End mock is added.
gRPC interceptor and transaction manager tests internal/database/database_tx_interceptor.go, internal/database/database_tx_interceptor_test.go, internal/database/database_tx_manager_test.go	The gRPC unary server interceptor calls tx.End(ctx) directly instead of i.manager.End(ctx, tx). Interceptor tests update expectations from manager mock to transaction mock across all behavior cases. Transaction manager tests remove gomock controller usage and update "commit" and "rollback" scenarios to call tx.End(ctx) directly; cases verifying unsupported transaction types or cross-manager transactions are removed.
DAO and database layer test updates internal/database/dao/generic_dao*_test.go, internal/database/database_listener_test.go, internal/database/database_notifier_test.go	All test helper functions (runWithTx, DeferCleanup, notify) switch from tm.End(ctx, tx) to tx.End(ctx) for transaction finalization. Updates applied consistently across events, immutability, integrity, lock, metrics, tenancy, and version DAO tests.
Server implementations and test fixtures internal/servers/console_server.go, internal/servers/console_server_test.go, internal/servers/cluster_templates_server_test.go, internal/servers/clusters_server_test.go, internal/servers/compute_instance_catalog_items_server_test.go, internal/servers/compute_instance_templates_server_test.go, internal/servers/public_ip_pools_server_test.go, internal/servers/servers_suite_test.go, internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go	Console server's resolveComputeInstance method updates deferred cleanup to call tx.End(ctx). Server test fixtures (BeforeEach and DeferCleanup blocks) universally switch to direct tx.End(ctx) calls. Console server test mock gains End method returning nil. Start gRPC server command updates kubeconfig hub config provider transaction cleanup.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Complexity: Moderate refactoring with homogeneous pattern changes across many test files, but involving a meaningful API contract change that must be verified at the core transaction manager level.

Risk Assessment: ⚠️ Low severity, localized impact. The refactoring moves transaction completion responsibility onto the transaction object itself, which is logically sound (objects managing their own lifecycle). No external API changes; only internal database and server code affected. The pattern is consistent and testable—all ~44 call-site updates follow the same mechanical replacement. However, the critical checkpoint is verifying that managedTx.End(ctx) correctly implements the commit/rollback decision logic and error collection that was previously in TxManager.End. Interceptor and manager tests provide coverage for the new path.

Possibly related PRs

• osac-project/fulfillment-service#16: Introduced the gRPC transactions interceptor and TxManager.End(ctx, tx) lifecycle that this PR directly refactors.
• osac-project/fulfillment-service#635: Modified the shared servers test harness transaction setup in internal/servers/servers_suite_test.go—the main PR updates the same file's deferred cleanup logic.

Suggested labels

approved, lgtm

Suggested reviewers

• eranco74
• akshaynadkarni
• larsks

Poem

Transactions now manage their own end,
No longer routed through the manager's hand.
From tm.End to tx.End we transcend,
Cleaner ownership, refactored as planned. ✨

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and specifically describes the primary change: moving the End method from the TxManager interface to the Tx interface, which is directly reflected in the 26 file changes throughout the codebase.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	No hardcoded secrets detected. PR contains only transaction management refactoring with no new credentials, API keys, tokens, passwords, or sensitive information.
No-Weak-Crypto	✅ Passed	No weak cryptography detected. PR refactors transaction management (End method) with zero crypto-related changes; no MD5, SHA1, DES, RC4, Blowfish, or ECB usage found.
No-Injection-Vectors	✅ Passed	PR refactors End() method from TxManager to Tx interface. All SQL queries use parameterized style ($1,$2), no string concatenation, eval, shell commands, or other injection vectors detected.
Container-Privileges	✅ Passed	PR contains only Go source code changes; no container/K8s manifests with privilege configurations are modified. Check is not applicable.
No-Sensitive-Data-In-Logs	✅ Passed	PR moves End() method from TxManager to Tx interface. New managedTx.End() logs database operation errors at DEBUG level; codebase confirms sensitive credentials are not stored in database.
Ai-Attribution	✅ Passed	Commit includes proper Red Hat attribution trailer "Assisted-by: Cursor" for AI-assisted work; Co-Authored-By not misused.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}