remove go-bits/easypg - #1123
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR updates ChangesPostgreSQL URL migration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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)
80-88: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPreserve the old
application_namebehavior on new PostgreSQL URIs.
easypg.URLFrompreviously setapplication_name, butpgruntime.ConnectionTarget.IntoURL()only mergesConnectionOptionsandExtraConnectionOptions; it does not add it automatically. The production and PostgreSQL-container targets now produce URLs without that session metadata.
internal/knowledge/db/db.go#L80-L88: carry the old application name intoExtraConnectionOptionsor add an equivalent explicit option for production connections.internal/knowledge/db/testing/env.go#L31-L39: preserve the same option so the test path stays consistent unless the difference is intentional.🤖 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/knowledge/db/db.go` around lines 80 - 88, The PostgreSQL targets no longer preserve the previous application_name session metadata. Update the ConnectionTarget setup in internal/knowledge/db/db.go:80-88 to add the legacy application name through ExtraConnectionOptions or an equivalent explicit connection option, and apply the same change in internal/knowledge/db/testing/env.go:31-39 so production and test connections remain consistent.Source: MCP tools
🤖 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.
Nitpick comments:
In `@internal/knowledge/db/db.go`:
- Around line 80-88: The PostgreSQL targets no longer preserve the previous
application_name session metadata. Update the ConnectionTarget setup in
internal/knowledge/db/db.go:80-88 to add the legacy application name through
ExtraConnectionOptions or an equivalent explicit connection option, and apply
the same change in internal/knowledge/db/testing/env.go:31-39 so production and
test connections remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 209ad27a-c2fa-4f20-99e0-10de621a87c9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modinternal/knowledge/db/db.gointernal/knowledge/db/testing/env.go
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@go.mod`:
- Line 13: Inspect the module graph and repository imports for any remaining use
of github.com/sapcc/go-bits; if none remain, remove its go.mod requirement and
update go.sum accordingly, while preserving the existing go.xyrillian.de/gg
dependency and any transitively required entries.
🪄 Autofix
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: Pro Plus
Run ID: b46c5de9-9b64-431c-b310-5268e1cec0de
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modinternal/knowledge/db/db.gointernal/knowledge/db/testing/env.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/knowledge/db/db.go
- internal/knowledge/db/testing/env.go
I am replacing go-bits/easypg with a clean-room reimplementation, gg/pgruntime, taht features several improvements over the original API (listed e.g. in [1]). None of those are really relevant for you since Cortex uses barely anything from easypg, but since I would like to get rid of go-bits/easypg eventually, this replaces your usage of the API with the equivalent API from gg/pgruntime. [1]: sapcc/keppel#766 Signed-off-by: Stefan Majewsky <stefan.majewsky@sap.com>
1 participant
I am replacing go-bits/easypg with a clean-room reimplementation, gg/pgruntime, that features several improvements over the original API (listed e.g. in sapcc/keppel#766). None of those are really relevant for you since Cortex uses barely anything from easypg, but since I would like to get rid of go-bits/easypg eventually, this replaces your usage of the API with the equivalent API from gg/pgruntime.
If you will allow me a side note (which is not relevant for the scope of this PR), I had to chuckle when I saw that you are using sqlite for your unit tests. I did the same in Limes and Castellum all those years ago, until I finally had enough of having to deal with two separate database dialects, and realized that spawning a PostgreSQL server is barely any slower, and in fact can make CI faster because it removes the compile time for the go-sqlite CGo stuff. If you are interested in getting rid of some useless code paths, have a look at https://pkg.go.dev/go.xyrillian.de/gg/pgruntime#WithTestDB. (You will also need to patch your Makefile and GitHub Actions workflows to install PostgreSQL etc.; you can find what to do by grepping for
if sr.UsesPostgresin the go-makefile-maker source code.)