fix: migrations rerunning on every start for logstore - #4421
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 selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughTwo Postgres connection-target log statements are downgraded from ChangesPostgres Framework Changes
EnvLabel Truncation and Test Config
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Confidence Score: 4/5Merge should wait for the schema contract to match the runtime config loader behavior. The changed files are small and the migration/logging updates are straightforward, but the published config schema remains inconsistent with the new runtime label limit and can reject configs the application now accepts. transports/config.schema.json should be updated to match transports/bifrost-http/lib/config.go.
 T-Rex LogsWhat T-Rex did
Reviews (3): Last reviewed commit: "Update config.go" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tests/integrations/python/config.json`:
- Line 3: The env_label value in the config.json fixture exceeds the maximum
length constraint defined in the transport schema. The value "Development" is 11
characters, but transports/config.schema.json specifies maxLength of 10 for
env_label. Shorten the env_label value to 10 characters or fewer to align with
the schema constraint that serves as the source of truth for transport
configuration.
In `@transports/bifrost-http/lib/config.go`:
- Around line 889-893: The truncateLabel function in
transports/bifrost-http/lib/config.go has a logic error: it checks if the rune
length exceeds 10 but then truncates to 14 runes instead of 10. This contradicts
the transports/config.schema.json schema which enforces maxLength: 10 for
env_label as the authoritative source of truth. Fix the function by changing the
return statement to return only the first 10 runes (string(r[:10])) to align the
runtime behavior with the schema constraint.
- Around line 891-893: The truncation logic in the code has a boundary mismatch
that causes a panic. The condition checks if len(r) > 10 (meaning length is at
least 11), but then attempts to slice r[:14], which will panic when the actual
length is 11, 12, or 13. Fix this by changing the boundary check to len(r) > 14
to ensure the slice operation is safe, or alternatively by clamping the slice
index to the actual length of the rune slice using a minimum function before
accessing r[:14].
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b8b94f8-3fce-487f-9247-bd12689048bc
📒 Files selected for processing (5)
framework/configstore/postgres.goframework/logstore/migrations.goframework/logstore/postgres.gotests/integrations/python/config.jsontransports/bifrost-http/lib/config.go
c361ce6 to
37f1865
Compare
Merge activity
|
| if len(r) > 14 { | ||
| return string(r[:14]) |
There was a problem hiding this comment.
The loader now accepts and truncates env_label at 14 runes, and this PR adds the 11-character value Development, but transports/config.schema.json still advertises and enforces maxLength: 10. Configs with labels from 11 to 14 characters can now load successfully at runtime but fail schema validation in tools or CI that use the published schema. Please update the schema description and maxLength to 14 with this runtime limit.
Rule Used: transports/config.schema.json is the source of tru... (source)
## Summary This PR addresses a few minor correctness and noise issues: migration dialect guards are moved inside the transaction callback so they are properly recorded in the migrations table even when skipped, Postgres connection log lines are demoted from Info to Debug to reduce log noise, the env label truncation limit is increased, and a development label is added to the Python integration test config. ## Changes - Moved the `db.Dialector.Name() != "postgres"` early-return checks from outside the migrator setup into the `Migrate` callback for `migrationSplitFilterDataMatView`, `migrationAddSafeJsonbFunction`, `migrationRecreateFilterUsersMatView`, `migrationRecreateFilterTeamBUMatViews`, and `migrationRecreateFilterCustomersMatView`. This ensures non-Postgres environments still register the migration as applied rather than silently skipping it, preventing re-runs on future startups. The same guard was removed entirely from `migrationRecreateMatViewsWithGovernanceColumns` (the migrator itself handles the no-op). - Downgraded the Postgres connection target log lines in both `configstore` and `logstore` from `Info` to `Debug` to reduce routine startup noise. - Increased the `truncateLabel` character limit from 10 to 14 in the HTTP transport config loader to allow slightly longer labels without truncation. - Added `"env_label": "Development"` to the Python integration test config. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/configstore/... ./framework/logstore/... ./transports/bifrost-http/... ``` Verify that on a non-Postgres dialect, the previously skipped migrations are now recorded as applied in the migrations table after startup. Confirm that Postgres connection details no longer appear at the Info log level during startup. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations The Postgres connection log demotion to Debug reduces the chance of host, port, and database name appearing in Info-level log aggregators, which is a minor improvement for environments where Info logs are broadly exported. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
3 participants
Summary
This PR addresses a few minor correctness and noise issues: migration dialect
guards are moved inside the transaction callback so they are properly recorded
in the migrations table even when skipped, Postgres connection log lines are
demoted from Info to Debug to reduce log noise, the env label truncation limit
is increased, and a development label is added to the Python integration test
config.
Changes
db.Dialector.Name() != "postgres"early-return checks from outsidethe migrator setup into the
Migratecallback formigrationSplitFilterDataMatView,migrationAddSafeJsonbFunction,migrationRecreateFilterUsersMatView,migrationRecreateFilterTeamBUMatViews, andmigrationRecreateFilterCustomersMatView. This ensures non-Postgresenvironments still register the migration as applied rather than silently
skipping it, preventing re-runs on future startups. The same guard was removed
entirely from
migrationRecreateMatViewsWithGovernanceColumns(the migratoritself handles the no-op).
configstoreandlogstorefromInfotoDebugto reduce routine startup noise.truncateLabelcharacter limit from 10 to 14 in the HTTPtransport config loader to allow slightly longer labels without truncation.
"env_label": "Development"to the Python integration test config.Type of change
Affected areas
How to test
go test ./framework/configstore/... ./framework/logstore/... ./transports/bifrost-http/...Verify that on a non-Postgres dialect, the previously skipped migrations are now
recorded as applied in the migrations table after startup. Confirm that Postgres
connection details no longer appear at the Info log level during startup.
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
The Postgres connection log demotion to Debug reduces the chance of host, port,
and database name appearing in Info-level log aggregators, which is a minor
improvement for environments where Info logs are broadly exported.
Checklist
docs/contributing/README.mdand followed the guidelines