feat: migrate naked flat fee lines #2901
Conversation
📝 WalkthroughWalkthroughThis change introduces a migration and supporting infrastructure to convert legacy flat fee billing invoice lines to usage-based pricing (UBP) flat fee lines. It adds SQL migration scripts, SQLC-generated Go database access code, test fixtures, and comprehensive tests to verify the migration. Documentation and configuration files for SQLC and test data generation are also included. Changes
Possibly related PRs
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 (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
turip
force-pushed
the
feat/use-ubp-lines-subs-sync
branch
from
459091f to
38c8878
Compare
turip
changed the title
turip
force-pushed
the
feat/migrate-naked-flat-fee-lines
branch
from
2a3f1a5 to
eb42c2f
Compare
turip
force-pushed
the
feat/migrate-naked-flat-fee-lines
branch
8 times, most recently
from
1277c00 to
7739ec8
Compare
turip
force-pushed
the
feat/migrate-naked-flat-fee-lines
branch
from
7739ec8 to
fffb2a0
Compare
turip
force-pushed
the
feat/migrate-naked-flat-fee-lines
branch
from
fffb2a0 to
1362808
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🧹 Nitpick comments (8)
tools/migrate/testdata/billing/README.md (5)
3-3: Minor style improvement: Consider more concise wording.Consider rewording for better clarity and conciseness.
-Given billing depends on a lot of things (such as customer, billing profile, subscription etc.), it's easier to generate data using tests. +Given billing depends on many entities (such as customer, billing profile, subscription etc.), it's easier to generate data using tests.🧰 Tools
🪛 LanguageTool
[style] ~3-~3: Consider using a synonym to be more concise.
Context: ...ration checks Given billing depends on a lot of things (such as customer, billing profi...(A_LOT_OF)
5-5: Minor style improvement: Remove redundant phrase.-We cannot use ent here, as the schema evolves, so we need a point in time snapshot. +We cannot use ent here, as the schema evolves, so we need a point-in-time snapshot.🧰 Tools
🪛 LanguageTool
[style] ~5-~5: This phrase is redundant. Consider writing “point” or “time”.
Context: ...re, as the schema evolves, so we need a point in time snapshot. SQLC is a great codegenerator...(MOMENT_IN_TIME)
11-11: Fix grammar: Use "a" instead of "an" before "unit".-Write an unit test that generates the required data. +Write a unit test that generates the required data.🧰 Tools
🪛 LanguageTool
[misspelling] ~11-~11: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...# How to generate a new testcase Write an unit test that generates the required d...(EN_A_VS_AN)
18-29: Add language specification to code block.The markdown linter flagged missing language specification.
-``` +```yaml version: "2" sql: - engine: "postgresql"🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
18-18: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
42-76: Fix formatting and syntax issues in Go code example.The code example has several issues:
- Hard tabs should be replaced with spaces
- Syntax errors in the Go code structure
```go package migrate_test import ( - "context" - "database/sql" - "encoding/json" - "os" - "strings" - "testing" + "context" + "database/sql" + "encoding/json" + "os" + "strings" + "testing" - flatfeetoubpflatfeedb "github.com/openmeterio/openmeter/tools/migrate/testdata/billing/flatfeetoubpflatfee/db" + flatfeetoubpflatfeedb "github.com/openmeterio/openmeter/tools/migrate/testdata/billing/flatfeetoubpflatfee/db" ) func TestMigrateFlatFeesToUBPFlatFees(t *testing.T) { - runner{stops{ - { - version: 20250527084817, - direction: directionUp, - action: func(t *testing.T, db *sql.DB) { - loadFixture(t, db, "testdata/billing/flatfeetoubpflatfee/fixture.sql") - }, - }, - { - version: 20250527123425, - direction: directionUp, - action: func(t *testing.T, db *sql.DB) { - q := flatfeetoubpflatfeedb.New(db) + runner{stops{ + { + version: 20250527084817, + direction: directionUp, + action: func(t *testing.T, db *sql.DB) { + loadFixture(t, db, "testdata/billing/flatfeetoubpflatfee/fixture.sql") + }, + }, + { + version: 20250527123425, + direction: directionUp, + action: func(t *testing.T, db *sql.DB) { + q := flatfeetoubpflatfeedb.New(db) - // execute queries - }, - }, - }}.Test(t) + // execute queries + }, + }, + }}.Test(t) }🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
46-46: Hard tabs
Column: 1(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1(MD010, no-hard-tabs)
59-59: Hard tabs
Column: 1(MD010, no-hard-tabs)
60-60: Hard tabs
Column: 1(MD010, no-hard-tabs)
61-61: Hard tabs
Column: 1(MD010, no-hard-tabs)
62-62: Hard tabs
Column: 1(MD010, no-hard-tabs)
63-63: Hard tabs
Column: 1(MD010, no-hard-tabs)
64-64: Hard tabs
Column: 1(MD010, no-hard-tabs)
65-65: Hard tabs
Column: 1(MD010, no-hard-tabs)
66-66: Hard tabs
Column: 1(MD010, no-hard-tabs)
67-67: Hard tabs
Column: 1(MD010, no-hard-tabs)
68-68: Hard tabs
Column: 1(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1(MD010, no-hard-tabs)
tools/migrate/testdata/billing/flatfeetoubpflatfee/db/helpers.go (1)
28-30: Consider removing redundant validation.The validation
ubpLine.ID != lineParent.Stringappears redundant sinceGetUsageBasedLineByIDwas just called withlineParent.Stringas the ID parameter. If the query succeeds, the IDs should match by design.This validation might be unnecessary unless there's a specific reason to expect ID mismatches. Consider removing it to simplify the code:
- if ubpLine.ID != lineParent.String { - return LineHierarchy{}, fmt.Errorf("ubp line id does not match line parent id") - } -tools/migrate/billing_test.go (2)
93-95: Fix comment inconsistencies in assertions.The comments don't match the actual assertions:
- Line 93: Comment says "should be 1" but assertion checks for "0"
- Line 95: Comment says "should be 1" but assertion checks for "0"
- require.Equal(t, ubpLine.PreLinePeriodQuantity.String, "0", "pre line period quantity should be 1") + require.Equal(t, ubpLine.PreLinePeriodQuantity.String, "0", "pre line period quantity should be 0") require.Equal(t, ubpLine.MeteredQuantity.String, "1", "metered quantity should be 1") - require.Equal(t, ubpLine.MeteredPreLinePeriodQuantity.String, "0", "metered pre line period quantity should be 1") + require.Equal(t, ubpLine.MeteredPreLinePeriodQuantity.String, "0", "metered pre line period quantity should be 0")
136-146: Consider enhancing the fixture loading function.The
loadFixturefunction is basic and doesn't parse SQL syntax. While it works for this use case, consider documenting any limitations or edge cases it might have with complex SQL fixtures.
🛑 Comments failed to post (5)
tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.down.sql (1)
1-1: 🛠️ Refactor suggestion
Consider providing automated rollback logic or clearer manual instructions.
While the complexity of reversing flat fee to UBP migrations may justify manual intervention, this approach has several concerns:
- Breaks automated workflows: CI/CD pipelines expect reversible migrations
- Production risk: Manual rollbacks in production are error-prone
- Incomplete guidance: The comment doesn't specify the manual steps needed
Consider either:
- Implementing automated rollback using the backup table data
- Providing detailed manual rollback procedures in documentation
- Adding validation checks to prevent rollback in problematic states
-- Example automated rollback approach: -- 1. Restore original flat fee lines from billing_backup_migrated_flat_fees -- 2. Remove generated UBP lines -- 3. Validate data consistency🤖 Prompt for AI Agents
In tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.down.sql at line 1, the current manual revert comment lacks clear rollback instructions and automation, which risks errors and breaks CI/CD expectations. To fix this, implement an automated rollback script that restores original flat fee lines from the billing_backup_migrated_flat_fees table, deletes the generated UBP lines, and includes validation checks for data consistency. Alternatively, provide detailed manual rollback steps in the comment or documentation to guide safe reversal.tools/migrate/testdata/billing/flatfeetoubpflatfee/db/helpers.go (2)
32-35:
⚠️ Potential issueUse the provided context parameter instead of context.Background().
Same context inconsistency issue as above.
- detailedLines, err := q.GetFlatFeeLinesByParentID(context.Background(), lineParent.String) + detailedLines, err := q.GetFlatFeeLinesByParentID(ctx, lineParent.String)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.detailedLines, err := q.GetFlatFeeLinesByParentID(ctx, lineParent.String) if err != nil { return LineHierarchy{}, err }🤖 Prompt for AI Agents
In tools/migrate/testdata/billing/flatfeetoubpflatfee/db/helpers.go around lines 32 to 35, replace the use of context.Background() with the context parameter passed into the function to maintain context consistency. Modify the call to q.GetFlatFeeLinesByParentID to use the provided context argument instead of creating a new background context.
23-26:
⚠️ Potential issueUse the provided context parameter instead of context.Background().
The method accepts a
ctxparameter but usescontext.Background()for the database call, which bypasses any timeouts, cancellations, or tracing associated with the original context.- ubpLine, err := q.GetUsageBasedLineByID(context.Background(), lineParent.String) + ubpLine, err := q.GetUsageBasedLineByID(ctx, lineParent.String)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.ubpLine, err := q.GetUsageBasedLineByID(ctx, lineParent.String) if err != nil { return LineHierarchy{}, err }🤖 Prompt for AI Agents
In tools/migrate/testdata/billing/flatfeetoubpflatfee/db/helpers.go around lines 23 to 26, replace the use of context.Background() with the provided ctx parameter in the call to q.GetUsageBasedLineByID. This ensures that the database call respects the original context's timeouts, cancellations, and tracing.tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.up.sql (2)
119-123:
⚠️ Potential issueFix metadata handling logic.
The current logic checks if metadata equals
'null'::jsonb(a JSON null value), but this seems incorrect. It should likely check for SQL NULL instead.- IF existing_flat_fee.metadata = 'null'::jsonb THEN + IF existing_flat_fee.metadata IS NULL THEN metadata_in = '{}'::jsonb; ELSE metadata_in = coalesce(existing_flat_fee.metadata, '{}'::jsonb); END IF;The current condition would only match if the metadata column contains the JSON value
null, not if it's SQL NULL. This could lead to incorrect metadata handling during migration.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.IF existing_flat_fee.metadata IS NULL THEN metadata_in = '{}'::jsonb; ELSE metadata_in = coalesce(existing_flat_fee.metadata, '{}'::jsonb); END IF;🤖 Prompt for AI Agents
In tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.up.sql around lines 119 to 123, the condition incorrectly checks if metadata equals the JSON null value instead of checking for SQL NULL. Replace the condition to check if existing_flat_fee.metadata IS NULL to properly handle SQL NULL values, ensuring correct metadata assignment during migration.
223-223: 💡 Verification agent
🧩 Analysis chain
Verify migration scope and performance impact.
The migration runs on all valid flat fee lines at once. For large datasets, this could be resource-intensive.
Consider whether this migration should:
- Run in batches for better performance
- Include progress logging
- Have rollback procedures documented
🏁 Script executed:
#!/bin/bash # Check if there are any performance considerations or batch processing patterns in other migrations rg -A 10 -B 5 "SELECT.*FROM.*WHERE.*AND" tools/migrate/migrations/ | head -50Length of output: 738
Ensure efficient and safe flat‐fee migration
I didn’t find any existing migrations using batched processing in tools/migrate/migrations—this step:
SELECT pg_temp.migrate_flat_fees_to_ubp_flat_fees(id) FROM billing_invoice_lines WHERE type = 'flat_fee' AND status = 'valid';will touch all qualifying rows in one go, which on large tables can lead to long locks and high resource usage. Please consider:
- Breaking the work into batches (e.g. LIMIT/OFFSET loops or server-side cursors)
- Emitting progress logs (e.g. RAISE NOTICE)
- Documenting rollback steps or adding a corresponding DOWN migration
Location to update:
• tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.up.sql, line 223🤖 Prompt for AI Agents
In tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.up.sql at line 223, the migration runs on all valid flat fee invoice lines in a single query, which can cause performance issues on large datasets. Refactor this to process the rows in batches using LIMIT and OFFSET or a server-side cursor to reduce locking and resource usage. Add RAISE NOTICE statements to log progress after each batch. Also, document rollback steps or create a corresponding DOWN migration to safely revert the changes if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tools/migrate/testdata/billing/README.md (6)
3-4: Avoid vague phrasing.
The phrase “depends on a lot of things” is imprecise. Consider specifying the key dependencies directly or rephrasing for clarity (e.g., “billing depends on customer, billing profile, subscription, etc.”).🧰 Tools
🪛 LanguageTool
[style] ~3-~3: Consider using a synonym to be more concise.
Context: ...ration checks Given billing depends on a lot of things (such as customer, billing profi...(A_LOT_OF)
5-6: Refine redundant phrase.
“point in time snapshot” is a bit redundant. You can hyphenate for idiomatic usage: “point-in-time snapshot” or simply “snapshot.”🧰 Tools
🪛 LanguageTool
[style] ~5-~5: This phrase is redundant. Consider writing “point” or “time”.
Context: ...re, as the schema evolves, so we need a point in time snapshot. SQLC is a great codegenerator...(MOMENT_IN_TIME)
7-7: Correct verb agreement.
Change “what have happened” to “what happened” or “what has happened” to fix the verb agreement.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ed to tweak the test to understand what have happened with a faulty migration. ## H...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
11-11: Fix article usage.
Use “a unit test” instead of “an unit test” for correct English.Apply this diff:
- Write an unit test that generates the required data. Make it fail with `t.FailNow()` so that the database snapshot is retained. + Write a unit test that generates the required data. Make it fail with `t.FailNow()` so that the database snapshot is retained.🧰 Tools
🪛 LanguageTool
[misspelling] ~11-~11: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...# How to generate a new testcase Write an unit test that generates the required d...(EN_A_VS_AN)
18-18: Add language identifier to code fence.
The YAML snippet is introduced with backticks but lacks a language tag. Change to “```yaml” for proper highlighting.Apply this diff:
- ``` + ```yaml🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
18-18: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
1-9: Consider adding a Prerequisites section.
It could help readers to know upfront which tools to install (e.g.,pg_dump, Postgres instance,sqlc) and any environment variables required before following the steps.🧰 Tools
🪛 LanguageTool
[style] ~3-~3: Consider using a synonym to be more concise.
Context: ...ration checks Given billing depends on a lot of things (such as customer, billing profi...(A_LOT_OF)
[style] ~5-~5: This phrase is redundant. Consider writing “point” or “time”.
Context: ...re, as the schema evolves, so we need a point in time snapshot. SQLC is a great codegenerator...(MOMENT_IN_TIME)
[uncategorized] ~7-~7: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ed to tweak the test to understand what have happened with a faulty migration. ## H...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtools/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
flake.nix(1 hunks)go.mod(1 hunks)tools/migrate/billing_test.go(1 hunks)tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.down.sql(1 hunks)tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.up.sql(1 hunks)tools/migrate/testdata/billing/README.md(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/db/db.go(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/db/helpers.go(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/db/models.go(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/db/queries.sql.go(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/fixture.sql(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/sqlc.yaml(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/sqlc/queries.sql(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- go.mod
- tools/migrate/testdata/billing/flatfeetoubpflatfee/sqlc/queries.sql
- tools/migrate/testdata/billing/flatfeetoubpflatfee/fixture.sql
🚧 Files skipped from review as they are similar to previous changes (9)
- tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.down.sql
- tools/migrate/testdata/billing/flatfeetoubpflatfee/db/helpers.go
- tools/migrate/testdata/billing/flatfeetoubpflatfee/db/models.go
- flake.nix
- tools/migrate/testdata/billing/flatfeetoubpflatfee/db/db.go
- tools/migrate/testdata/billing/flatfeetoubpflatfee/sqlc.yaml
- tools/migrate/testdata/billing/flatfeetoubpflatfee/db/queries.sql.go
- tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.up.sql
- tools/migrate/billing_test.go
🧰 Additional context used
🪛 LanguageTool
tools/migrate/testdata/billing/README.md
[style] ~3-~3: Consider using a synonym to be more concise.
Context: ...ration checks Given billing depends on a lot of things (such as customer, billing profi...
(A_LOT_OF)
[style] ~5-~5: This phrase is redundant. Consider writing “point” or “time”.
Context: ...re, as the schema evolves, so we need a point in time snapshot. SQLC is a great codegenerator...
(MOMENT_IN_TIME)
[uncategorized] ~7-~7: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ed to tweak the test to understand what have happened with a faulty migration. ## H...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[misspelling] ~11-~11: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...# How to generate a new testcase Write an unit test that generates the required d...
(EN_A_VS_AN)
🪛 markdownlint-cli2 (0.17.2)
tools/migrate/testdata/billing/README.md
18-18: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1
(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
59-59: Hard tabs
Column: 1
(MD010, no-hard-tabs)
60-60: Hard tabs
Column: 1
(MD010, no-hard-tabs)
61-61: Hard tabs
Column: 1
(MD010, no-hard-tabs)
62-62: Hard tabs
Column: 1
(MD010, no-hard-tabs)
63-63: Hard tabs
Column: 1
(MD010, no-hard-tabs)
64-64: Hard tabs
Column: 1
(MD010, no-hard-tabs)
65-65: Hard tabs
Column: 1
(MD010, no-hard-tabs)
66-66: Hard tabs
Column: 1
(MD010, no-hard-tabs)
67-67: Hard tabs
Column: 1
(MD010, no-hard-tabs)
68-68: Hard tabs
Column: 1
(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (1)
tools/migrate/testdata/billing/README.md (1)
46-69: Hard tabs in Go snippet can be ignored.
The markdownlint warnings about hard tabs are false positives here—Go code uses tabs by convention. To suppress these messages, you can add a<!-- markdownlint-disable MD010 -->comment above the snippet.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
46-46: Hard tabs
Column: 1(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1(MD010, no-hard-tabs)
59-59: Hard tabs
Column: 1(MD010, no-hard-tabs)
60-60: Hard tabs
Column: 1(MD010, no-hard-tabs)
61-61: Hard tabs
Column: 1(MD010, no-hard-tabs)
62-62: Hard tabs
Column: 1(MD010, no-hard-tabs)
63-63: Hard tabs
Column: 1(MD010, no-hard-tabs)
64-64: Hard tabs
Column: 1(MD010, no-hard-tabs)
65-65: Hard tabs
Column: 1(MD010, no-hard-tabs)
66-66: Hard tabs
Column: 1(MD010, no-hard-tabs)
67-67: Hard tabs
Column: 1(MD010, no-hard-tabs)
68-68: Hard tabs
Column: 1(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1(MD010, no-hard-tabs)
turip
force-pushed
the
feat/migrate-naked-flat-fee-lines
branch
2 times, most recently
from
b356a65 to
7e34ee7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tools/migrate/testdata/billing/README.md (4)
3-4: Split “testcases” into two words for clarity
Use “test cases” instead of the single-word “testcases” to align with common usage.- Given billing depends on many entities (such as customer, billing profile, subscription etc.), it's easier to generate data using tests. This guide shows how we can create testcases based on sql dumps for faster validation. + Given billing depends on many entities (such as customer, billing profile, subscription, etc.), it's easier to generate data using tests. This guide shows how we can create test cases based on SQL dumps for faster validation.
5-7: Fix verb agreement and wording
The phrase “understand what have happened” should be “understand what has happened” and clarify the event context. Also expand “db” to “database”.- I would just commit those alongside the tests in case we would need to tweak the test to understand what have happened with a faulty migration. + I would commit those alongside the tests in case we need to tweak them to understand what has happened during a faulty migration.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ed to tweak the test to understand what have happened with a faulty migration. ## H...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
11-12: Clarify test-failure intent
Rephrase to make it clear thatt.FailNow()halts the test to preserve the database snapshot.- Write a unit test that generates the required data. Make it fail with `t.FailNow()` so that the database snapshot is retained. + Write a unit test that generates the required data and call `t.FailNow()` to stop the test immediately, preserving the database snapshot for dumping.
46-69: Replace hard tabs with spaces in fenced code blocks
Markdownlint is flagging hard tabs within this Go snippet. Convert all leading tabs to spaces (e.g., 2 or 4 spaces per indent) to satisfy the no-hard-tabs rule.- import ( - "context" - ... + import ( + "context" + ...🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
46-46: Hard tabs
Column: 1(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1(MD010, no-hard-tabs)
59-59: Hard tabs
Column: 1(MD010, no-hard-tabs)
60-60: Hard tabs
Column: 1(MD010, no-hard-tabs)
61-61: Hard tabs
Column: 1(MD010, no-hard-tabs)
62-62: Hard tabs
Column: 1(MD010, no-hard-tabs)
63-63: Hard tabs
Column: 1(MD010, no-hard-tabs)
64-64: Hard tabs
Column: 1(MD010, no-hard-tabs)
65-65: Hard tabs
Column: 1(MD010, no-hard-tabs)
66-66: Hard tabs
Column: 1(MD010, no-hard-tabs)
67-67: Hard tabs
Column: 1(MD010, no-hard-tabs)
68-68: Hard tabs
Column: 1(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtools/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
flake.nix(1 hunks)go.mod(1 hunks)tools/migrate/billing_test.go(1 hunks)tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.down.sql(1 hunks)tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.up.sql(1 hunks)tools/migrate/testdata/billing/README.md(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/db/db.go(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/db/helpers.go(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/db/models.go(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/db/queries.sql.go(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/fixture.sql(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/sqlc.yaml(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/sqlc/queries.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/migrate/testdata/billing/flatfeetoubpflatfee/sqlc/queries.sql
🚧 Files skipped from review as they are similar to previous changes (11)
- go.mod
- tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.down.sql
- tools/migrate/testdata/billing/flatfeetoubpflatfee/sqlc.yaml
- tools/migrate/testdata/billing/flatfeetoubpflatfee/db/models.go
- flake.nix
- tools/migrate/testdata/billing/flatfeetoubpflatfee/db/db.go
- tools/migrate/testdata/billing/flatfeetoubpflatfee/db/helpers.go
- tools/migrate/testdata/billing/flatfeetoubpflatfee/fixture.sql
- tools/migrate/migrations/20250527123425_migrate-flat-fees-to-ubp-flat-fees.up.sql
- tools/migrate/billing_test.go
- tools/migrate/testdata/billing/flatfeetoubpflatfee/db/queries.sql.go
🧰 Additional context used
🪛 LanguageTool
tools/migrate/testdata/billing/README.md
[uncategorized] ~7-~7: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ed to tweak the test to understand what have happened with a faulty migration. ## H...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🪛 markdownlint-cli2 (0.17.2)
tools/migrate/testdata/billing/README.md
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1
(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
59-59: Hard tabs
Column: 1
(MD010, no-hard-tabs)
60-60: Hard tabs
Column: 1
(MD010, no-hard-tabs)
61-61: Hard tabs
Column: 1
(MD010, no-hard-tabs)
62-62: Hard tabs
Column: 1
(MD010, no-hard-tabs)
63-63: Hard tabs
Column: 1
(MD010, no-hard-tabs)
64-64: Hard tabs
Column: 1
(MD010, no-hard-tabs)
65-65: Hard tabs
Column: 1
(MD010, no-hard-tabs)
66-66: Hard tabs
Column: 1
(MD010, no-hard-tabs)
67-67: Hard tabs
Column: 1
(MD010, no-hard-tabs)
68-68: Hard tabs
Column: 1
(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Analyze (go)
| @@ -0,0 +1,76 @@ | |||
| # Billing migration checks | |||
There was a problem hiding this comment.
we discussed it that it would be nice to generalize this solution for future migration tests as the current way of writing raw SQL queries for both seeding & assertions is a PITA
turip
force-pushed
the
feat/migrate-naked-flat-fee-lines
branch
from
7e34ee7 to
4a9e7ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tools/migrate/testdata/billing/README.md (1)
14-15: Remove hard-coded credentials
Embedding real DB credentials in documentation risks exposing sensitive information. Replace with placeholders (e.g.,<username>,<password>) or reference environment variables.
🧹 Nitpick comments (5)
tools/migrate/testdata/billing/README.md (2)
7-7: Fix verb agreement
The phrase "what have happened" should be "what has happened" for correct subject-verb agreement.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ed to tweak the test to understand what have happened with a faulty migration. ## H...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
46-69: Replace hard tabs with spaces
Hard tabs in code blocks can break formatting and violate Markdown style rules. Convert these to spaces (e.g., 2 or 4 spaces) for consistent indentation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
46-46: Hard tabs
Column: 1(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1(MD010, no-hard-tabs)
59-59: Hard tabs
Column: 1(MD010, no-hard-tabs)
60-60: Hard tabs
Column: 1(MD010, no-hard-tabs)
61-61: Hard tabs
Column: 1(MD010, no-hard-tabs)
62-62: Hard tabs
Column: 1(MD010, no-hard-tabs)
63-63: Hard tabs
Column: 1(MD010, no-hard-tabs)
64-64: Hard tabs
Column: 1(MD010, no-hard-tabs)
65-65: Hard tabs
Column: 1(MD010, no-hard-tabs)
66-66: Hard tabs
Column: 1(MD010, no-hard-tabs)
67-67: Hard tabs
Column: 1(MD010, no-hard-tabs)
68-68: Hard tabs
Column: 1(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1(MD010, no-hard-tabs)
tools/migrate/migrations/20250605131637_migrate-flat-fees-to-ubp-flat-fees.up.sql (3)
109-113: Prefer jsonb_build_object over manual JSON formatting
Usingformat(... )::jsonbis error-prone and harder to read. Use native constructors:- format('{"type": "flat", "amount": "%s", "paymentTerm": "%s"}', - existing_flat_fee.per_unit_amount, - existing_flat_fee.payment_term - )::jsonb, + jsonb_build_object( + 'type', 'flat', + 'amount', existing_flat_fee.per_unit_amount, + 'paymentTerm', existing_flat_fee.payment_term + ),
67-74: Implement a backup or down migration
Relying on commented suggestions for backups leaves no safety net. Consider creating a backup table or adding a rollback script before transforming data for safer migrations.
75-75: Use snake_case for function parameters
idToMigratebreaks conventional naming in SQL/PLpgSQL. Rename toid_to_migrate(or prefix withp_) for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtools/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
flake.nix(1 hunks)go.mod(1 hunks)tools/migrate/billing_test.go(1 hunks)tools/migrate/migrations/20250605131637_migrate-flat-fees-to-ubp-flat-fees.down.sql(1 hunks)tools/migrate/migrations/20250605131637_migrate-flat-fees-to-ubp-flat-fees.up.sql(1 hunks)tools/migrate/testdata/billing/README.md(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/db/db.go(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/db/helpers.go(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/db/models.go(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/db/queries.sql.go(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/fixture.sql(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/sqlc.yaml(1 hunks)tools/migrate/testdata/billing/flatfeetoubpflatfee/sqlc/queries.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/migrate/migrations/20250605131637_migrate-flat-fees-to-ubp-flat-fees.down.sql
🚧 Files skipped from review as they are similar to previous changes (10)
- go.mod
- flake.nix
- tools/migrate/testdata/billing/flatfeetoubpflatfee/db/models.go
- tools/migrate/testdata/billing/flatfeetoubpflatfee/sqlc.yaml
- tools/migrate/testdata/billing/flatfeetoubpflatfee/fixture.sql
- tools/migrate/testdata/billing/flatfeetoubpflatfee/db/db.go
- tools/migrate/testdata/billing/flatfeetoubpflatfee/sqlc/queries.sql
- tools/migrate/billing_test.go
- tools/migrate/testdata/billing/flatfeetoubpflatfee/db/queries.sql.go
- tools/migrate/testdata/billing/flatfeetoubpflatfee/db/helpers.go
🧰 Additional context used
🪛 LanguageTool
tools/migrate/testdata/billing/README.md
[uncategorized] ~7-~7: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ed to tweak the test to understand what have happened with a faulty migration. ## H...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🪛 markdownlint-cli2 (0.17.2)
tools/migrate/testdata/billing/README.md
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1
(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
59-59: Hard tabs
Column: 1
(MD010, no-hard-tabs)
60-60: Hard tabs
Column: 1
(MD010, no-hard-tabs)
61-61: Hard tabs
Column: 1
(MD010, no-hard-tabs)
62-62: Hard tabs
Column: 1
(MD010, no-hard-tabs)
63-63: Hard tabs
Column: 1
(MD010, no-hard-tabs)
64-64: Hard tabs
Column: 1
(MD010, no-hard-tabs)
65-65: Hard tabs
Column: 1
(MD010, no-hard-tabs)
66-66: Hard tabs
Column: 1
(MD010, no-hard-tabs)
67-67: Hard tabs
Column: 1
(MD010, no-hard-tabs)
68-68: Hard tabs
Column: 1
(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Developer environment
- GitHub Check: Quickstart
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Analyze (go)
2 participants
Overview
This patch adds a migration that:
Given I need to generate ulids and do complex transformations plpgsql functions are used, in the
pg_tempschema, which only exists during the lifecycle of this sessionl.Given the complexity of test setup, an sqlc based approach is used for migrations.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores