Add MigrationContext.Hooks for in-process hook implementations#1675
Conversation
gh-ost's only hook extension point is on-disk scripts globbed from --hooks-path. Library callers that embed Migrator must either ship scripts and their dependencies alongside their binary or maintain a parallel Go layer that bridges script side effects back into the host application. Introduce a Hooks interface in go/base with one method per lifecycle event, and an optional MigrationContext.Hooks field. NewMigrator reads the field once at construction and falls back to the existing HooksExecutor when unset, so CLI behavior is unchanged. A CompositeHooks helper in go/logic lets callers run the on-disk script executor and their own Go implementation side-by-side. HooksExecutor's previously package-private method names are renamed (onStartup -> OnStartup, etc.) so external types can satisfy the interface. The struct and constructor were already exported but the methods weren't, so no usable external API is displaced.
olsonjp
requested review from
meiji163,
rashiq and
timvaillancourt
as code owners
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in in-process hooks extension point for library consumers of logic.NewMigrator, allowing hook lifecycle events to be handled by Go implementations (and optionally composed with the existing --hooks-path shell script executor) while keeping CLI/default behavior unchanged.
Changes:
- Introduces
base.Hooksand addsMigrationContext.Hooksas an optional override for hook handling. - Updates the migrator/server to depend on the
base.Hooksinterface, defaulting tologic.HooksExecutorwhen no hooks are provided. - Adds
logic.CompositeHooksplus unit tests and documentation for embedded usage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| go/logic/server.go | Switches server hook dependency from *HooksExecutor to base.Hooks and calls exported hook methods. |
| go/logic/migrator.go | Reads MigrationContext.Hooks once during construction, defaulting to the shell-based executor when unset. |
| go/logic/hooks.go | Adds CompositeHooks and exports HooksExecutor lifecycle methods to satisfy base.Hooks. |
| go/logic/hooks_test.go | Adds tests for CompositeHooks ordering/short-circuiting and NewMigrator hook selection. |
| go/base/hooks.go | Defines the new base.Hooks interface. |
| go/base/context.go | Adds Hooks base.Hooks to MigrationContext. |
| doc/hooks.md | Documents embedded usage via MigrationContext.Hooks and composing with logic.CompositeHooks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func NewMigrator(context *base.MigrationContext, appVersion string) *Migrator { | ||
| hooks := context.Hooks | ||
| if hooks == nil { | ||
| hooks = NewHooksExecutor(context) | ||
| } |
There was a problem hiding this comment.
Declined. MigrationContext.Log (interface Logger) is the same pattern in this codebase: swap-able by library callers, used as mctx.Log.Infof(...) throughout without nil checks, and exposed to the same typed-nil hazard. Defending against typed-nil here would be inconsistent with the existing style.
| func (c CompositeHooks) OnStartup() error { | ||
| for _, h := range c { | ||
| if err := h.OnStartup(); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 0927d5f — each fan-out method now skips nil members, with a TestCompositeHooks_SkipsNil test.
|
|
||
| ctx.Hooks = &myHooks{} |
There was a problem hiding this comment.
Fixed in 0927d5f — snippet now declares version and constructs ctx via base.NewMigrationContext().
Address PR review feedback: - CompositeHooks.OnX methods skip nil members instead of panicking, allowing callers to conditionally append optional hooks. - doc/hooks.md embedded-usage snippet now defines ctx and version so it is self-contained.
|
Great feature, thanks! As for the location of the Hooks interface and exporting the hook methods, I am good with both as they are in your PR 👍 |
Related issue: #1674
Description
This PR adds an additive extension point that lets library callers register Go hook implementations instead of (or alongside) the on-disk script contract behind
--hooks-path.Hooksinterface ingo/basewith one method per lifecycle event.MigrationContext.Hooksfield.logic.NewMigratorreads it once at construction and falls back to the existingHooksExecutorwhen unset, so default behavior is unchanged for CLI users.logic.CompositeHooksfor callers who want to layer Go callbacks on top of the on-disk script executor.HooksExecutormethod names (onStartup→OnStartup, etc.) so external types can satisfy the interface. The struct and constructor were already exported, but the methods weren't, so this isn't displacing any usable external API.Open questions
A couple of design choices I'd appreciate input on:
Hooksinterface belong? The PR puts it ingo/basenext toHooksPath/HooksHintMessage/HooksHintOwner/HooksHintToken, which already act as the hook-config home onMigrationContext. Living ingo/logicnext toHooksExecutoris also defensible — but would either requireMigrationContextto hold the field asanyand type-assert, or invert the import direction.go/logic, so I don't believe this displaces real usage — but happy to take a different shape (e.g. keeponXxxand add exported shims) if you'd rather leave the existing names alone.Tests
TestHooksExecutorExecuteHooks(and itsdoes-not-exist/failed/successsubtests covering the CLI path) is unchanged and still passes.CompositeHooksordering and short-circuit semantics, andNewMigratorhonoringMigrationContext.Hooks.script/docker-gh-ost-replica-tests) passes (88/88, 1 sysbench skip).