feat(reconcile): add BillingProduct kind - #1816
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds billing product reconciliation support: price normalization, validation, diff planning, API integration, export, and CLI registration. The changes ensure consistent immutability comparisons and supply valid defaults for metered prices. ChangesBilling product lifecycle
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
Coverage Report for CI Build 30618661356Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.4%) to 47.685%Details
Uncovered Changes
Coverage Regressions45 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
baccf42 to
51e421b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal/reconcile/billingproduct_test.go (1)
186-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a subtest for the
metered_aggregatedefault in the diff.The case-difference tests cover currency, interval, and usage type. No test covers the
"sum"default thatnormalizeBillingPriceapplies.billing/product/service_test.gocovers this at the service layer, butinternal/reconcileholds its own copy of the defaults, so a drift there would not fail any test.💚 Proposed subtest
t.Run("does not plan a change when a metered price omits its aggregate", func(t *testing.T) { cur := curToken() cur.Prices = []BillingPriceSpec{{ Name: "default", Amount: 100, Currency: "usd", Interval: "month", UsageType: "metered", BillingScheme: "flat", MeteredAggregate: "sum", }} s := newBillingProduct() s.Prices[0].UsageType = "metered" // metered_aggregate omitted; "sum" is the default ops, err := diffBillingProducts([]BillingProductSpec{s}, []currentBillingProduct{cur}) assert.NoError(t, err) assert.Empty(t, ops) })The
validateBillingProductSpectable also has no case for a feature with an empty name. Consider adding{"empty feature name", func(s *BillingProductSpec) { s.Features[0].Name = "" }, "feature with no name"}.internal/reconcile/billingproduct.go (1)
372-392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReference the
billing/productdefault constants instead of duplicating literals.
"licensed"and"flat"are already exported asproduct.PriceUsageTypeLicensedandproduct.BillingSchemeFlat; centralize all four defaults so changes inbilling/productdo not drift frominternal/reconcile/billingproduct.go.internal/reconcile/billingproduct_reconciler.go (2)
265-299: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused error return from
billingProductBody.
billingProductBodynever returns a non-nil error. The two call sites add an error branch that cannot execute. Return only*frontierv1beta1.ProductRequestBodyand remove the branches.
303-305: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCompare the price state case-insensitively.
hasTieredPricelowercases the billing scheme read from the server, butbillingPriceStateActivecompares the raw state. A state of"Active"would then be treated as retired, which would keep the price out of the export and change the diff. Use the same normalization for both server-read fields.♻️ Proposed change
func billingPriceStateActive(state string) bool { - return state == "" || state == "active" + s := strings.ToLower(strings.TrimSpace(state)) + return s == "" || s == "active" }internal/reconcile/billingproduct_reconciler_test.go (1)
319-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn an error for an unknown product id in the fake.
UpdateProductlooks upf.byID[req.Msg.GetId()]and writes topwithout a nil check. If the reconciler ever sends an id the fake does not hold, the test panics with a nil dereference instead of reporting the wrong id.♻️ Proposed change
p := f.byID[req.Msg.GetId()] + if p == nil { + return nil, fmt.Errorf("unknown product id %q", req.Msg.GetId()) + } b := req.Msg.GetBody()
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d396e028-e7e8-4c12-bf68-366a14a612c7
⛔ Files ignored due to path filters (2)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go,!proto/**proto/v1beta1/models.pb.gois excluded by!**/*.pb.go,!proto/**
📒 Files selected for processing (13)
Makefilebilling/product/service.gobilling/product/service_test.gocmd/reconcile.godocs/content/docs/reconcile.mdxgo.modinternal/api/v1beta1connect/billing_product.gointernal/api/v1beta1connect/billing_product_test.gointernal/reconcile/billingproduct.gointernal/reconcile/billingproduct_reconciler.gointernal/reconcile/billingproduct_reconciler_test.gointernal/reconcile/billingproduct_test.gotest/e2e/regression/billing_test.go
💤 Files with no reviewable changes (1)
- test/e2e/regression/billing_test.go
whoAbhishekSah
left a comment
whoAbhishekSah
left a comment
There was a problem hiding this comment.
Went through the new kind. A few comments below, nothing blocking.
…licated enum lists
…rice does not false-reject on update
…he plan is deterministic
…to during dry run
…s for billing product
…drop metadata from update and export
…nge, skip products the kind cannot represent
…n, out-of-scope products left alone
…ling product instead of planning a doomed create
…ce the tiered scheme from product, rename shadowed vars
…ort, remove billingProductBody's unused error, note the ListProducts single-page assumption
…ure name; use range-over-int
35df3f1 to
8a37e7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e852a44b-6105-401a-8a80-03f4989e51b8
⛔ Files ignored due to path filters (2)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go,!proto/**proto/v1beta1/models.pb.gois excluded by!**/*.pb.go,!proto/**
📒 Files selected for processing (13)
Makefilebilling/product/service.gobilling/product/service_test.gocmd/reconcile.godocs/content/docs/reconcile.mdxgo.modinternal/api/v1beta1connect/billing_product.gointernal/api/v1beta1connect/billing_product_test.gointernal/reconcile/billingproduct.gointernal/reconcile/billingproduct_reconciler.gointernal/reconcile/billingproduct_reconciler_test.gointernal/reconcile/billingproduct_test.gotest/e2e/regression/billing_test.go
💤 Files with no reviewable changes (1)
- test/e2e/regression/billing_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/api/v1beta1connect/billing_product.go
- internal/api/v1beta1connect/billing_product_test.go
- Makefile
- go.mod
- cmd/reconcile.go
- internal/reconcile/billingproduct_test.go
- billing/product/service.go
- billing/product/service_test.go
- internal/reconcile/billingproduct_reconciler.go
- internal/reconcile/billingproduct_reconciler_test.go
- internal/reconcile/billingproduct.go
4 participants
What
Adds a
BillingProductreconcile kind. It manages billing products — their title, description, behavior, config, prices, and features — from a desired-state YAML file, using the product APIs (ListProducts,CreateProduct,UpdateProduct). Part of the declarative reconcile flow in RFC 0001, one kind per resource.How it follows the RFC's five rules
name, a price bynamewithin its product, a feature byname; identity is never rewritten. Provider ids, timestamps, price state, and metadata are out of scope. A product this kind cannot represent (a tiered price, an empty title, or a name under three characters) is left untouched, and a file that names it fails the plan.title,description, andconfigare full-write; a missing title fails the plan (the provider uses it as the product name and requires it). Behavior is create-only, so a change to it fails the plan. Prices converge: a new name is added, a retired name is reactivated when its fields match, an omitted active price is retired; an immutable-field change fails the plan. A product missing from the file fails the plan;delete: trueis rejected (there is no product delete API).Validate()runs each entry server-free, including the proto's own enum and format rules viaprotovalidate, so a bad value fails the whole file up front rather than at apply.Depends on (all merged)
ProductRequestBody), both merged and pinned.Notes
Testing
Unit-tested at the validate, diff, and reconciler layers (convergence, retire/reactivate, immutable-change and out-of-scope failures, the export round-trip). Verified end to end against a local server plus a Stripe mock, and reviewed by an independent pass against all five rules. Build, tests, gofmt, vet, and lint are green.