Skip to content

update runsinglemigration to accept migration options - #3626

Merged
akshaydeo merged 1 commit into
devfrom
05-20-update_runsinglemigration_to_accept_migration_options
May 20, 2026
Merged

akshaydeo merged 1 commit into
devfrom
05-20-update_runsinglemigration_to_accept_migration_options

Conversation

@akshaydeo

@akshaydeo akshaydeo commented May 20, 2026

Copy link
Copy Markdown
Contributor

Summary

RunSingleMigration previously always used migrator.DefaultOptions, making it impossible for callers to supply custom migrator options. This PR adds an optional *migrator.Options parameter so downstream consumers (e.g. bifrost-enterprise, plugins) can pass their own options when needed, while all existing internal callers pass nil to retain the default behavior.

Changes

  • Added an options *migrator.Options parameter to RunSingleMigration; when nil is passed, migrator.DefaultOptions is used as before
  • Updated all internal call sites to pass nil, preserving existing behavior

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (React)
  • Docs

How to test

go test ./framework/configstore/...

Verify that all existing migrations continue to run successfully with nil options, and that a custom *migrator.Options value is correctly applied when provided.

Breaking changes

  • Yes
  • No

RunSingleMigration is a public function. Its signature has changed — callers outside this repository (e.g. bifrost-enterprise, plugins) must add a nil (or custom options) argument as the second parameter.

Related issues

N/A

Security considerations

None. This change only affects how migrator options are threaded through to the migration runner.

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fbcb96a7-3be0-430b-8a24-58b31d00c282

📥 Commits

Reviewing files that changed from the base of the PR and between 78778fb and 0074ae3.

📒 Files selected for processing (1)
  • framework/configstore/migrations.go

📝 Walkthrough

Summary by CodeRabbit

  • Refactor
    • Updated internal database migration handling for improved configuration flexibility.

Walkthrough

RunSingleMigration in framework/configstore/migrations.go now accepts a *migrator.Options parameter between ctx and db. When options is nil, it defaults to migrator.DefaultOptions. Eight migration functions are updated to call the function with nil for the new options parameter, preserving existing behavior.

Changes

Migration Options Parameter

Layer / File(s) Summary
RunSingleMigration signature and implementation
framework/configstore/migrations.go
RunSingleMigration accepts a new *migrator.Options parameter, defaults to migrator.DefaultOptions when nil, and constructs the migrator with those options before executing the migration.
Migration call site updates
framework/configstore/migrations.go
Eight migration functions (migrationAddTeamSourceIDColumn, migrationUniqueTeamNames, migrationAddOAuthAuthModeColumns, migrationReplaceOauthSessionTokenWithSessionID, migrationDropLegacyOAuthServerTables, migrationDropNonVKOauthUserRows, migrationDropMCPExternalServerURL, migrationRefreshConfigHashAfterMCPExternalServerURLRemoval) update their RunSingleMigration calls to pass nil for options using the new parameter order.

🎯 2 (Simple) | ⏱️ ~12 minutes

🐰 A function spreads its wings so wide,
Now options flow in, side by side,
Eight migrations hop along the way,
All passing nil to save the day! 🌱

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 05-20-update_runsinglemigration_to_accept_migration_options

Comment @coderabbitai help to get the list of available commands and usage tips.

@akshaydeo
akshaydeo marked this pull request as ready for review May 20, 2026 11:39
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

akshaydeo commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

akshaydeo commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

Merge activity

  • May 20, 11:39 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • May 20, 11:39 AM UTC: @akshaydeo merged this pull request with Graphite.

@akshaydeo
akshaydeo merged commit f0b695b into dev May 20, 2026
13 of 15 checks passed
@akshaydeo
akshaydeo deleted the 05-20-update_runsinglemigration_to_accept_migration_options branch May 20, 2026 11:39
@greptile-apps

greptile-apps Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 4/5

Safe to merge; the only concern is a latent pointer-aliasing issue that does not cause any current misbehaviour.

The change is small and all internal callers are correctly updated. The one thing worth watching is that migrationOpts := migrator.DefaultOptions copies the pointer to the global struct rather than a fresh value. migrator.New fills in zero-value fields in-place on the struct it receives, so if a new Options field is ever added without a corresponding default, the first nil-options call would silently mutate the process-wide DefaultOptions and affect all subsequent migrations.

framework/configstore/migrations.go — specifically the migrationOpts fallback assignment around line 154.

Important Files Changed

Filename Overview
framework/configstore/migrations.go Adds an options *migrator.Options parameter to RunSingleMigration; all internal callers updated to pass nil. When nil is provided, the code assigns the global migrator.DefaultOptions pointer directly rather than making a struct copy, which could corrupt the global if migrator.New ever mutates a new zero-value field.

Reviews (1): Last reviewed commit: "update runsinglemigration to accept migr..." | Re-trigger Greptile

Comment on lines +154 to +157
migrationOpts := migrator.DefaultOptions
if options != nil {
migrationOpts = options
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 When options is nil, migrationOpts is assigned the same pointer as the global migrator.DefaultOptions. migrator.New mutates the *Options it receives to fill in any zero-value fields in-place. Today DefaultOptions is fully populated so no mutation fires, but if a new field is added to Options without a corresponding default, New would corrupt the package-level global on the first call, affecting every subsequent migration for the lifetime of the process. Copying the struct value avoids this aliasing risk entirely.

Suggested change
migrationOpts := migrator.DefaultOptions
if options != nil {
migrationOpts = options
}
defaultCopy := *migrator.DefaultOptions
migrationOpts := &defaultCopy
if options != nil {
migrationOpts = options
}

akhsaul pushed a commit to akhsaul/bifrost that referenced this pull request Aug 27, 2026
## Summary

`RunSingleMigration` previously always used `migrator.DefaultOptions`, making it impossible for callers to supply custom migrator options. This PR adds an optional `*migrator.Options` parameter so downstream consumers (e.g. bifrost-enterprise, plugins) can pass their own options when needed, while all existing internal callers pass `nil` to retain the default behavior.

## Changes

- Added an `options *migrator.Options` parameter to `RunSingleMigration`; when `nil` is passed, `migrator.DefaultOptions` is used as before
- Updated all internal call sites to pass `nil`, preserving existing behavior

## Type of change

- [ ] Bug fix
- [ ] Feature
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

```sh
go test ./framework/configstore/...
```

Verify that all existing migrations continue to run successfully with `nil` options, and that a custom `*migrator.Options` value is correctly applied when provided.

## Breaking changes

- [x] Yes
- [ ] No

`RunSingleMigration` is a public function. Its signature has changed — callers outside this repository (e.g. bifrost-enterprise, plugins) must add a `nil` (or custom options) argument as the second parameter.

## Related issues

N/A

## Security considerations

None. This change only affects how migrator options are threaded through to the migration runner.

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
occcat pushed a commit to occcat/bifrost that referenced this pull request Sep 2, 2026
## Summary

`RunSingleMigration` previously always used `migrator.DefaultOptions`, making it impossible for callers to supply custom migrator options. This PR adds an optional `*migrator.Options` parameter so downstream consumers (e.g. bifrost-enterprise, plugins) can pass their own options when needed, while all existing internal callers pass `nil` to retain the default behavior.

## Changes

- Added an `options *migrator.Options` parameter to `RunSingleMigration`; when `nil` is passed, `migrator.DefaultOptions` is used as before
- Updated all internal call sites to pass `nil`, preserving existing behavior

## Type of change

- [ ] Bug fix
- [ ] Feature
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

```sh
go test ./framework/configstore/...
```

Verify that all existing migrations continue to run successfully with `nil` options, and that a custom `*migrator.Options` value is correctly applied when provided.

## Breaking changes

- [x] Yes
- [ ] No

`RunSingleMigration` is a public function. Its signature has changed — callers outside this repository (e.g. bifrost-enterprise, plugins) must add a `nil` (or custom options) argument as the second parameter.

## Related issues

N/A

## Security considerations

None. This change only affects how migrator options are threaded through to the migration runner.

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants