Merged
Conversation
SoWieMarkus
requested review from
PhilippMatthes,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
📝 WalkthroughWalkthroughRefined transaction error handling in the knowledge DB to return wrapped, contextual errors and ensure explicit rollbacks; added a test that verifies rollback on duplicate-key insert. Multiple OpenStack syncer tests were updated to call Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/knowledge/db/db.go (1)
151-153: Consider extracting rollback+log into a helper to avoid drift.The same rollback logging block is repeated in three places. A tiny helper would keep this behavior consistent.
♻️ Suggested refactor
+func rollbackWithLog(tx *gorp.Transaction) { + if rbErr := tx.Rollback(); rbErr != nil { + slog.Error("failed to rollback transaction", "error", rbErr) + } +} + func (d *DB) CreateTable(table ...*gorp.TableMap) error { @@ if _, err := tx.Exec(sql); err != nil { - if rbErr := tx.Rollback(); rbErr != nil { - slog.Error("failed to rollback transaction", "error", rbErr) - } + rollbackWithLog(tx) return fmt.Errorf("failed to create table %s: %w", t.TableName, err) } } @@ if _, err = tx.Exec("DELETE FROM " + tableName); err != nil { - if rbErr := tx.Rollback(); rbErr != nil { - slog.Error("failed to rollback transaction", "error", rbErr) - } + rollbackWithLog(tx) return fmt.Errorf("failed to delete old objects from %s: %w", tableName, err) } if err = BulkInsert(tx, db, objs...); err != nil { - if rbErr := tx.Rollback(); rbErr != nil { - slog.Error("failed to rollback transaction", "error", rbErr) - } + rollbackWithLog(tx) return fmt.Errorf("failed to insert new objects into %s: %w", tableName, err) }Also applies to: 209-211, 215-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/knowledge/db/db.go` around lines 151 - 153, The repeated tx rollback+log pattern (tx.Rollback() followed by slog.Error("failed to rollback transaction", "error", rbErr)) in internal/knowledge/db/db.go should be extracted into a small helper (e.g., rollbackWithLog or (db *DB).rollbackWithLog) that accepts the *sql.Tx (and logger or uses package slog) and performs the rollback and standardized logging; replace the three inline blocks that call tx.Rollback() and slog.Error with calls to this helper to ensure consistent behavior across the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/knowledge/db/db.go`:
- Around line 151-153: The repeated tx rollback+log pattern (tx.Rollback()
followed by slog.Error("failed to rollback transaction", "error", rbErr)) in
internal/knowledge/db/db.go should be extracted into a small helper (e.g.,
rollbackWithLog or (db *DB).rollbackWithLog) that accepts the *sql.Tx (and
logger or uses package slog) and performs the rollback and standardized logging;
replace the three inline blocks that call tx.Rollback() and slog.Error with
calls to this helper to ensure consistent behavior across the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9d9223c-c48e-4b29-8eac-27a3a150beb8
📒 Files selected for processing (2)
internal/knowledge/db/db.gointernal/knowledge/db/db_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/knowledge/datasources/plugins/openstack/limes/limes_sync_test.go (1)
165-177: Consider addingInit()call toTestLimesSyncer_SyncCommitments_NoProjectsfor consistency.This test still uses the old pattern (creates API with
NewLimesAPI, then reassignssyncer.API) and doesn't callsyncer.Init(ctx)beforeSyncCommitments(). While this may work because the test expects an error anyway, it's inconsistent with the initialization pattern established in this PR.♻️ Suggested refactor for consistency
syncer := &LimesSyncer{ DB: testDB, Mon: mon, Conf: conf, - API: NewLimesAPI(mon, k, conf), + API: &mockLimesAPI{}, + } + + if err := syncer.Init(t.Context()); err != nil { + t.Fatalf("failed to init limes syncer: %v", err) } - syncer.API = &mockLimesAPI{} ctx := t.Context()This also allows removing the unused
kvariable and potentially thetestlibKeystoneimport if no other tests in this file need it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/knowledge/datasources/plugins/openstack/limes/limes_sync_test.go` around lines 165 - 177, The test TestLimesSyncer_SyncCommitments_NoProjects creates a LimesSyncer with NewLimesAPI then overwrites syncer.API and never calls syncer.Init(ctx), which is inconsistent with the rest of the tests; update the test to call syncer.Init(ctx) before calling SyncCommitments (use t.Context() as ctx), remove the now-unused k variable and, if no other tests need it, drop the testlibKeystone import, and keep mockLimesAPI assignment to ensure the mocked API is used after initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/knowledge/datasources/plugins/openstack/limes/limes_sync_test.go`:
- Around line 165-177: The test TestLimesSyncer_SyncCommitments_NoProjects
creates a LimesSyncer with NewLimesAPI then overwrites syncer.API and never
calls syncer.Init(ctx), which is inconsistent with the rest of the tests; update
the test to call syncer.Init(ctx) before calling SyncCommitments (use
t.Context() as ctx), remove the now-unused k variable and, if no other tests
need it, drop the testlibKeystone import, and keep mockLimesAPI assignment to
ensure the mocked API is used after initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 454704c4-54f0-4543-9d7d-c82572cb6f79
📒 Files selected for processing (4)
internal/knowledge/datasources/plugins/openstack/cinder/cinder_sync_test.gointernal/knowledge/datasources/plugins/openstack/limes/limes_sync_test.gointernal/knowledge/datasources/plugins/openstack/manila/manila_sync_test.gointernal/knowledge/datasources/plugins/openstack/placement/placement_sync_test.go
PhilippMatthes
approved these changes
Mar 31, 2026
umswmayj
approved these changes
Mar 31, 2026
| if err != nil { | ||
| slog.Error("failed to begin transaction", "error", err) | ||
| return tx.Rollback() | ||
| return fmt.Errorf("failed to begin transaction: %w", err) |
Collaborator
There was a problem hiding this comment.
Same as above here w a Rollback. Probably ok but just fyi
Contributor
Test Coverage ReportTest Coverage 📊: 68.6% |
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
3 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.
No description provided.