quarterly budget - #5996
Merged
Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds quarterly budget reset configuration with a configurable quarter-start month. It adds ChangesQuarterly budget reset configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Configstore
participant TableBudget
participant Database
participant GenerateBudgetHash
Configstore->>TableBudget: Update quarterly reset configuration
TableBudget->>Database: Save ResetConfigJSON
Database-->>TableBudget: Load ResetConfigJSON
TableBudget-->>Configstore: Expose QuarterStartMonth
Configstore->>GenerateBudgetHash: Hash budget fields and quarter start
GenerateBudgetHash-->>Configstore: Return budget hash
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
akshaydeo
marked this pull request as ready for review
August 9, 2026 18:01
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@framework/configstore/migrations.go`:
- Around line 11033-11040: Update the migration containing the Rollback function
for TableBudget.reset_config_json to mark it explicitly non-rollbackable
according to the migration framework’s supported semantics, rather than dropping
the column silently. If the framework requires a rollback callback, return a
clear non-rollbackable error and document that dropping the column permanently
deletes fiscal-quarter settings.
In `@framework/configstore/tables/budget.go`:
- Around line 140-147: Update TableBudget.WindowStart and its quarterly
calendar-boundary helper to pass b.QuarterStartMonth() alongside ResetDuration
and now, ensuring configured fiscal starts such as April are honored. Add a test
covering both January- and April-start quarterly budgets with distinct
boundaries.
🪄 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: 2dfd9618-edb7-444c-8062-69f55c4d1fe9
📒 Files selected for processing (7)
framework/configstore/budgetresetconfighash_test.goframework/configstore/budgetresetconfigstore_test.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/budget.goframework/configstore/tables/budgetresetconfig_test.goframework/configstore/tables/utils.go
This was referenced Aug 9, 2026
akshaydeo
force-pushed
the
quarterly-budget-storage
branch
from
August 10, 2026 04:37
df5aa2b to
2377591
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/configstore/migrations_test.go (1)
2951-2958: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover existing rows and column nullability.
This test inserts
seedonly after the migration. It can pass if the migration backfills existing rows or addsreset_config_jsonas non-null with a default. Insert a legacy budget after dropping the column and before running the migration. Then verify that the row survives with aNULLreset_config_json, and inspect column metadata to confirm nullability.As per coding guidelines, migration tests should cover data-preservation edge paths. Based on learnings from the provided PR objective, this migration must add a nullable column without backfilling existing budgets.
🤖 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 `@framework/configstore/migrations_test.go` around lines 2951 - 2958, Extend the migration test around migrationAddBudgetResetConfigColumn by inserting a legacy TableBudget row after dropping reset_config_json and before running the migration. After migration, fetch that row and verify it remains present with a NULL reset_config_json value, then inspect the column metadata to confirm reset_config_json is nullable rather than required or defaulted.Source: Coding guidelines
🤖 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 `@framework/configstore/migrations_test.go`:
- Around line 2951-2958: Extend the migration test around
migrationAddBudgetResetConfigColumn by inserting a legacy TableBudget row after
dropping reset_config_json and before running the migration. After migration,
fetch that row and verify it remains present with a NULL reset_config_json
value, then inspect the column metadata to confirm reset_config_json is nullable
rather than required or defaulted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 09201b63-d194-4583-998d-c648d13e5367
📒 Files selected for processing (2)
framework/configstore/migrations.goframework/configstore/migrations_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- framework/configstore/migrations.go
akshaydeo
force-pushed
the
quarterly-budget-storage
branch
from
August 10, 2026 06:57
2377591 to
0c4b974
Compare
17 tasks
akshaydeo
force-pushed
the
quarterly-budget-storage
branch
from
August 10, 2026 21:38
0c4b974 to
436fb8e
Compare
Contributor
Author
Merge activity
|
atharvamhaske
pushed a commit
to atharvamhaske/bifrost
that referenced
this pull request
Aug 13, 2026
## Summary Budgets with a quarterly reset duration (`1Q`) previously had no way to configure which month begins Q1. This meant all quarterly budgets defaulted to a January fiscal year, making it impossible to model an April or October fiscal year start. This PR introduces a `BudgetResetConfig` struct persisted as a JSON blob (`reset_config_json`) on the `governance_budgets` table, allowing operators to specify a `quarter_start_month` per budget. ## Changes - Added `BudgetResetConfig` struct with a `QuarterStartMonth` field (1–12), persisted as `reset_config_json` via GORM `BeforeSave`/`AfterFind` hooks on `TableBudget`. - Added `QuarterStartMonth()` accessor on `TableBudget` that defaults to `time.January` on nil receivers, nil configs, and zero values, so all call sites can read it unconditionally. - Added `IsQuarterlyDuration()` helper and `"Q"` suffix support to `ParseDuration`, approximating one quarter as 90 days (matching how `"M"` approximates 30 days). - Updated `GenerateBudgetHash` to include the effective quarter start month in the digest for quarterly budgets, so editing the quarter definition in `config.json` is detected as a change. Non-quarterly budgets produce byte-identical digests to avoid spurious resyncs on upgrade. - The hash reads `QuarterStartMonth()` rather than the raw `ResetConfigJSON` blob, ensuring a budget parsed from `config.json` (no blob yet) and its persisted counterpart hash identically. - Added `migrationAddBudgetResetConfigColumn` to add the nullable `reset_config_json` column to existing `governance_budgets` tables with no backfill, preserving the January-default cadence of all pre-existing budgets. - `BeforeSave` validates that `ResetConfig` is only set on quarterly durations and that `QuarterStartMonth` is in range before serializing, so a rejected config never reaches the column. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/configstore/... ./framework/configstore/tables/... ``` Key test scenarios covered: - `TestGenerateBudgetHashUnchangedForNonQuarterlyBudgets` — upgrading does not trigger spurious config resyncs for non-quarterly budgets. - `TestGenerateBudgetHashAgreesAcrossConfigAndDatabaseSources` — a `config.json` budget and its persisted counterpart hash identically. - `TestGenerateBudgetHashTracksQuarterStartMonth` — editing the quarter start is detected as a change. - `TestMigrationAddsBudgetResetConfigColumnToExistingTable` — migration is idempotent on an already-provisioned table. - `TestExistingBudgetsKeepTheirCadenceAfterMigration` — pre-existing budgets read back with nil `ResetConfig` and a January quarter start. - `TestBudgetResetConfigSurvivesNestedPreload` — `AfterFind` fires on preloaded associations, ensuring cluster peers receive the correct quarter definition. ## Breaking changes - [ ] Yes - [x] No The migration is additive and nullable with no backfill. All existing budgets continue to operate with a January quarter start, which is identical to their previous behavior. ## Related issues ## Security considerations No auth, secrets, or PII implications. The `reset_config_json` column stores only a fiscal month integer. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
This was referenced Aug 13, 2026
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
1 participant
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.
Summary
Budgets with a quarterly reset duration (
1Q) previously had no way to configure which month begins Q1. This meant all quarterly budgets defaulted to a January fiscal year, making it impossible to model an April or October fiscal year start. This PR introduces aBudgetResetConfigstruct persisted as a JSON blob (reset_config_json) on thegovernance_budgetstable, allowing operators to specify aquarter_start_monthper budget.Changes
BudgetResetConfigstruct with aQuarterStartMonthfield (1–12), persisted asreset_config_jsonvia GORMBeforeSave/AfterFindhooks onTableBudget.QuarterStartMonth()accessor onTableBudgetthat defaults totime.Januaryon nil receivers, nil configs, and zero values, so all call sites can read it unconditionally.IsQuarterlyDuration()helper and"Q"suffix support toParseDuration, approximating one quarter as 90 days (matching how"M"approximates 30 days).GenerateBudgetHashto include the effective quarter start month in the digest for quarterly budgets, so editing the quarter definition inconfig.jsonis detected as a change. Non-quarterly budgets produce byte-identical digests to avoid spurious resyncs on upgrade.QuarterStartMonth()rather than the rawResetConfigJSONblob, ensuring a budget parsed fromconfig.json(no blob yet) and its persisted counterpart hash identically.migrationAddBudgetResetConfigColumnto add the nullablereset_config_jsoncolumn to existinggovernance_budgetstables with no backfill, preserving the January-default cadence of all pre-existing budgets.BeforeSavevalidates thatResetConfigis only set on quarterly durations and thatQuarterStartMonthis in range before serializing, so a rejected config never reaches the column.Type of change
Affected areas
How to test
go test ./framework/configstore/... ./framework/configstore/tables/...Key test scenarios covered:
TestGenerateBudgetHashUnchangedForNonQuarterlyBudgets— upgrading does not trigger spurious config resyncs for non-quarterly budgets.TestGenerateBudgetHashAgreesAcrossConfigAndDatabaseSources— aconfig.jsonbudget and its persisted counterpart hash identically.TestGenerateBudgetHashTracksQuarterStartMonth— editing the quarter start is detected as a change.TestMigrationAddsBudgetResetConfigColumnToExistingTable— migration is idempotent on an already-provisioned table.TestExistingBudgetsKeepTheirCadenceAfterMigration— pre-existing budgets read back with nilResetConfigand a January quarter start.TestBudgetResetConfigSurvivesNestedPreload—AfterFindfires on preloaded associations, ensuring cluster peers receive the correct quarter definition.Breaking changes
The migration is additive and nullable with no backfill. All existing budgets continue to operate with a January quarter start, which is identical to their previous behavior.
Related issues
Security considerations
No auth, secrets, or PII implications. The
reset_config_jsoncolumn stores only a fiscal month integer.Checklist
docs/contributing/README.mdand followed the guidelines