feat: v0.1.0 — golang-migrate + goose drivers, module types, steps, conformance suite#1
Conversation
… steps, conformance suite Tasks 22-26: scaffold new workflow-plugin-migrations repo with: - go.mod (github.com/GoCodeAlone/workflow v0.18.2) - .goreleaser.yaml: three builds (migrations, atlas-migrate, workflow-migrate) - .github/workflows: CI (postgres service) + release - pkg/driver: MigrationDriver adapter + in-process registry - internal/module_migrations.go: database.migrations module type - internal/module_driver.go: database.migration_driver module type - internal/steps: step.migrate_up/down/status/to pipeline steps - internal/golangmigrate: golang-migrate driver implementation - internal/goose: goose driver implementation - internal/atlasplugin: stub for Atlas driver (Task 27) - pkg/testharness: embedded-postgres + provided-DSN harness - pkg/conformance: exported Suite with 6 behavioural cases + corpus SQL - pkg/cli: shared Cobra root for wfctl migrate * / workflow-migrate - cmd/workflow-plugin-migrations: main binary (golang-migrate + goose + CLI) - cmd/workflow-plugin-atlas-migrate: atlas-only binary - cmd/workflow-migrate: standalone pre-deploy binary - plugin.json: declares moduleTypes, stepTypes, migrationDrivers, cliCommands All short tests pass. Integration tests skip when no Postgres is available and run in CI via the postgres service container. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…one-liners) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…kground() Addresses code-review issue: the Status method was discarding its ctx argument and using context.Background(), ignoring any deadline/cancel the caller set. Now provider.Status(ctx) correctly propagates context. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix 1 (internal/golangmigrate/driver.go): URL scheme rewrite used a hardcoded length check (dsn[:10]) to detect "postgres://", but that prefix is 11 chars — off-by-one meant pgx5:// substitution never fired, causing "unknown driver postgres" in CI. Replaced with strings.HasPrefix. Fix 2 (pkg/conformance): goose v3 expects combined SQL files with -- +goose Up/Down annotations, not paired .up.sql/.down.sql files (those are the golang-migrate format). Added corpus_goose/ with combined files and taught Suite.extractCorpus to select the right corpus by driver name. Also updated internal/goose/driver_test.go to use combined format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion Each New() call now creates a unique PostgreSQL schema (wfm_test_<ns>_<rand>) and appends search_path=<schema> to the DSN so all tables land in an isolated namespace. Close(t) drops the schema CASCADE. This eliminates cross-test relation-already-exists and dirty-database failures when multiple test files run against the same postgres instance. Also remove -- +goose StatementBegin/End wrapper from corpus_goose migration 2 since CREATE INDEX cannot run inside a transaction block. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR scaffolds the new workflow-plugin-migrations repository, adding a Workflow external plugin and standalone CLI for running Postgres database migrations via golang-migrate and goose, along with a conformance test suite and CI/release automation.
Changes:
- Added migration drivers (
internal/golangmigrate,internal/goose), module/step types, and a shared Cobra CLI (pkg/cli). - Introduced a Postgres test harness (
pkg/testharness) plus an exported conformance suite with embedded SQL corpus (pkg/conformance). - Added CI + release automation (GitHub Actions, GoReleaser) and initialized module metadata (
go.mod,plugin.json).
Reviewed changes
Copilot reviewed 33 out of 35 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin.json | Plugin manifest describing capabilities, module/step types, and CLI command surface. |
| go.mod | Initializes Go module + dependencies for workflow integration and migration libraries. |
| go.sum | Dependency checksum lockfile for reproducible builds. |
| pkg/driver/driver.go | Exposes driver/request/result aliases and a simple adapter wrapper around interfaces.MigrationDriver. |
| pkg/driver/registry.go | Adds an in-process registry for named migration drivers. |
| pkg/testharness/harness.go | Adds a Postgres harness with per-test schema isolation and embedded-postgres fallback. |
| pkg/conformance/suite.go | Implements a reusable driver conformance test matrix and corpus extraction from embedded files. |
| pkg/conformance/suite_test.go | Runs the conformance suite against the built-in golang-migrate and goose drivers. |
| pkg/conformance/corpus/001_create_users.up.sql | Paired up migration for the generic corpus. |
| pkg/conformance/corpus/001_create_users.down.sql | Paired down migration for the generic corpus. |
| pkg/conformance/corpus/002_add_indexes.up.sql | Generic corpus migration adding indexes. |
| pkg/conformance/corpus/002_add_indexes.down.sql | Generic corpus rollback for index creation. |
| pkg/conformance/corpus_goose/00001_create_users.sql | Goose-format combined Up/Down migration for corpus. |
| pkg/conformance/corpus_goose/00002_add_indexes.sql | Goose-format combined Up/Down index migration for corpus. |
| pkg/cli/root.go | Shared Cobra command tree used by wfctl migrate * and workflow-migrate. |
| internal/plugin.go | Main plugin provider wiring: manifest + module/step type factories. |
| internal/plugin_test.go | Unit tests validating manifest/module types/step types and unknown-type errors. |
| internal/module_driver.go | Implements database.migration_driver module (declares driver name). |
| internal/module_driver_test.go | Tests driver module config validation. |
| internal/module_migrations.go | Implements database.migrations module with InvokeMethod dispatch to drivers. |
| internal/module_migrations_test.go | Tests migrations module initialization and lifecycle. |
| internal/steps/base.go | Implements migration step types: up/down/status/to with config parsing + timeouts. |
| internal/steps/base_test.go | Basic step construction and missing-config error-path tests. |
| internal/golangmigrate/driver.go | Implements interfaces.MigrationDriver using golang-migrate + pgx/v5 backend. |
| internal/golangmigrate/driver_test.go | Integration-style tests for the golang-migrate driver using the harness. |
| internal/goose/driver.go | Implements interfaces.MigrationDriver using pressly/goose + pgx stdlib. |
| internal/goose/driver_test.go | Integration-style tests for the goose driver using the harness. |
| internal/atlasplugin/plugin.go | Stub atlas-only plugin provider (Task 27 placeholder). |
| cmd/workflow-plugin-migrations/main.go | Main plugin binary entrypoint (plugin + wfctl CLI provider). |
| cmd/workflow-plugin-atlas-migrate/main.go | Atlas-only plugin binary entrypoint. |
| cmd/workflow-migrate/main.go | Standalone CLI entrypoint reusing pkg/cli. |
| .github/workflows/ci.yml | CI workflow running build/vet/tests with a Postgres service container. |
| .github/workflows/release.yml | Tag-based release workflow invoking GoReleaser v2. |
| .goreleaser.yaml | GoReleaser configuration for building 3 binaries + archives/SBOM/checksums. |
| .gitignore | Ignores build artifacts (dist/, *.test, etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Names returns the sorted list of registered driver names. | ||
| func (r *Registry) Names() []string { | ||
| r.mu.RLock() | ||
| defer r.mu.RUnlock() | ||
| return r.names() |
There was a problem hiding this comment.
The comment says Names returns a sorted list, but names() returns map iteration order (unsorted). Sort the names slice before returning (or adjust the comment if ordering is intentionally undefined).
| if err := m.Steps(-steps); err != nil && !errors.Is(err, migrate.ErrNoChange) { | ||
| return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate down: %w", err) | ||
| } | ||
|
|
||
| after, _, _ := m.Version() | ||
| _ = before | ||
| _ = after | ||
|
|
||
| return interfaces.MigrationResult{ | ||
| Applied: []string{fmt.Sprintf("rolled back %d migration(s)", steps)}, |
There was a problem hiding this comment.
Down always returns a single summary string in Applied, even when no migrations were actually rolled back (e.g., when m.Steps(-steps) returns migrate.ErrNoChange). This causes callers to misreport what happened. Consider returning an empty Applied slice on no-op, and/or returning the specific reverted versions (similar to collectApplied, but in reverse).
| if err := m.Steps(-steps); err != nil && !errors.Is(err, migrate.ErrNoChange) { | |
| return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate down: %w", err) | |
| } | |
| after, _, _ := m.Version() | |
| _ = before | |
| _ = after | |
| return interfaces.MigrationResult{ | |
| Applied: []string{fmt.Sprintf("rolled back %d migration(s)", steps)}, | |
| if err := m.Steps(-steps); err != nil { | |
| if errors.Is(err, migrate.ErrNoChange) { | |
| return interfaces.MigrationResult{ | |
| Applied: []string{}, | |
| DurationMs: time.Since(start).Milliseconds(), | |
| }, nil | |
| } | |
| return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate down: %w", err) | |
| } | |
| after, _, _ := m.Version() | |
| applied := []string{} | |
| if before > after { | |
| applied = []string{fmt.Sprintf("rolled back %d migration(s)", before-after)} | |
| } | |
| return interfaces.MigrationResult{ | |
| Applied: applied, |
| if err != nil { | ||
| return fmt.Errorf("migrate down: %w", err) | ||
| } | ||
| fmt.Printf("Rolled back %d migration(s): %v\n", len(result.Applied), result.Applied) |
There was a problem hiding this comment.
The rollback count printed here is derived from len(result.Applied), but drivers are not guaranteed to return one Applied entry per rolled-back migration (and the current golang-migrate driver returns a single summary string). This can misreport the rollback count. Prefer printing the requested --steps value (or a dedicated count field) and treat Applied as display-only.
| fmt.Printf("Rolled back %d migration(s): %v\n", len(result.Applied), result.Applied) | |
| fmt.Printf("Rolled back %d migration(s): %v\n", steps, result.Applied) |
| }, | ||
| } | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
A timeout is computed and stored in req.Options.Timeout, but the module uses context.Background() and never applies the timeout to the driver calls. This means timeout in module config currently doesn’t bound runtime. Use context.WithTimeout (or pass through a context from the engine if available) and call cancel() when done.
| ctx := context.Background() | |
| ctx, cancel := context.WithTimeout(context.Background(), timeout) | |
| defer cancel() |
| if cfg.DSNEnv == "" && rawCfg["dsn"] == nil { | ||
| // Allow direct dsn for test usage | ||
| if v, ok := rawCfg["dsn"].(string); !ok || v == "" { | ||
| cfg.DSNEnv = "DATABASE_URL" | ||
| } |
There was a problem hiding this comment.
newMigrationsModule checks for rawCfg["dsn"] (implying a literal DSN is supported in module config), but migrationsModuleConfig has no dsn field and resolveDSN never consults module config—only method args or env. Either add a DSN field and use it in resolveDSN, or remove this config-side dsn handling to avoid misleading users/tests.
| if cfg.DSNEnv == "" && rawCfg["dsn"] == nil { | |
| // Allow direct dsn for test usage | |
| if v, ok := rawCfg["dsn"].(string); !ok || v == "" { | |
| cfg.DSNEnv = "DATABASE_URL" | |
| } | |
| if cfg.DSNEnv == "" { | |
| cfg.DSNEnv = "DATABASE_URL" |
| - arm64 | ||
| ldflags: | ||
| - -s -w | ||
| - -X main.version={{.Version}} |
There was a problem hiding this comment.
-X main.version={{.Version}} is set, but the main packages don’t define a version variable. This will make the goreleaser build fail for this binary. Remove this ldflag (the atlasplugin Version is already set) or add a matching var version string in the main package.
| - -X main.version={{.Version}} |
| // DriverRef is the name of a database.migration_driver module, OR a driver | ||
| // name (golang-migrate, goose) for inline driver configuration. | ||
| DriverRef string `json:"driver_ref"` | ||
| // Driver is the driver name when not using a module ref. |
There was a problem hiding this comment.
The config comment/docstring says driver_ref can be the name of a database.migration_driver module, but Init() currently only treats driver_ref as a literal driver name and there is no module-reference resolution. Either implement module lookup/resolution for driver_ref or update the comment/schema so users don’t assume cross-module references work.
| // DriverRef is the name of a database.migration_driver module, OR a driver | |
| // name (golang-migrate, goose) for inline driver configuration. | |
| DriverRef string `json:"driver_ref"` | |
| // Driver is the driver name when not using a module ref. | |
| // DriverRef is treated as a literal migration driver name for backward | |
| // compatibility. It does not resolve database.migration_driver modules. | |
| DriverRef string `json:"driver_ref"` | |
| // Driver is the explicit migration driver name (for example, | |
| // golang-migrate or goose). |
| // Start embedded Postgres on a random port to avoid conflicts. | ||
| port := uint32(15432 + rand.Intn(1000)) | ||
| ep = embeddedpostgres.NewDatabase(embeddedpostgres.DefaultConfig(). |
There was a problem hiding this comment.
Port selection for embedded Postgres uses rand.Intn, but math/rand is never seeded. This makes the chosen port deterministic across processes, which can lead to frequent port collisions when running tests in parallel locally. Seed a per-harness RNG (or use an OS-assigned free port via net.Listen(":0")) before selecting the port.
| - -s -w | ||
| - -X main.version={{.Version}} | ||
| - -X github.com/GoCodeAlone/workflow-plugin-migrations/internal.Version={{.Version}} |
There was a problem hiding this comment.
-X main.version={{.Version}} is set, but the main packages don’t define a version variable. go build fails when an -X ldflag targets a missing symbol. Remove this ldflag (the internal package Version is already set) or add a matching var version string in the main package.
| t.Fatalf("Up() error: %v", err) | ||
| } | ||
|
|
||
| stBefore, _ := d.Status(ctx, s.req()) |
There was a problem hiding this comment.
In testDownOne, the error from Status is ignored (stBefore, _ := ...). If Status fails, the test can pass/fail for the wrong reason or panic later. Capture and assert the error (using t.Fatalf) like the other cases in this suite do.
| stBefore, _ := d.Status(ctx, s.req()) | |
| stBefore, err := d.Status(ctx, s.req()) | |
| if err != nil { | |
| t.Fatalf("Status() before Down error: %v", err) | |
| } |
…x migration CREATE INDEX CONCURRENTLY cannot run inside a transaction block, which is how golang-migrate/v4 wraps each migration by default. The conformance suite tests driver behavior, not production indexing strategy, so plain CREATE INDEX is sufficient. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- registry: sort Names() return value (comment claimed sorted; now it is) - golangmigrate: Down() returns actual rolled-back version strings and empty slice on no-op (ErrNoChange), not a synthetic summary string - module_migrations: apply context.WithTimeout so driver calls respect the configured timeout instead of running on context.Background() indefinitely - module_migrations: simplify dsn_env defaulting — always default to DATABASE_URL; remove dead rawCfg["dsn"] check in constructor - module_migrations: clarify driver_ref docstring (module lookup not yet impl) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- cmd/workflow-plugin-migrations: add var version string for goreleaser ldflag
- cmd/workflow-plugin-atlas-migrate: add var version string for goreleaser ldflag
- testharness: replace rand.Intn port selection with net.Listen(":0") to
atomically acquire a free port and avoid collisions in parallel test processes
- conformance: handle Status() error in testDownOne instead of silently
discarding it with blank identifier
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eration golang-migrate has no built-in API to list pending migrations. Open the file source directly (migratefile.File.Open) and walk First/Next to collect versions that exceed the current applied version. This fixes TestConformance/StatusReflectsState which was failing because Pending was always empty. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 35 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sourceURL := "file://" + req.Source.Dir | ||
| return migrate.New(sourceURL, dsn) | ||
| } |
There was a problem hiding this comment.
The file source URL is built by string concatenation ("file://" + dir). This is not portable for paths needing URL escaping and is likely to fail on Windows (drive letters/backslashes). Consider constructing a proper file URL (e.g., filepath.ToSlash + url.PathEscape).
| for i := 0; i < 10; i++ { | ||
| req := s.req() | ||
| req.Options.Steps = 1 | ||
| _, err := d.Down(ctx, req) | ||
| if err != nil { |
There was a problem hiding this comment.
testDownAll hard-codes a maximum of 10 Down() iterations. If the embedded corpus grows beyond 10 migrations, the suite may stop early and incorrectly pass. Consider looping until Down indicates no more migrations (or deriving the max iterations from the corpus contents).
| res, err := provider.Down(ctx) | ||
| if err != nil { | ||
| // No more migrations to roll back. | ||
| break | ||
| } |
There was a problem hiding this comment.
In goose driver Down(), any error from provider.Down() is silently treated as "no more migrations" and the method returns success. This can mask real rollback failures. Consider only swallowing goose’s specific "no pending migrations" error and returning other errors.
| fmt.Printf("Migrated to %s (%d steps): %v\n", target, len(result.Applied), result.Applied) | ||
| return nil |
There was a problem hiding this comment.
The CLI prints "(%d steps)" using len(result.Applied), but drivers may not return one Applied entry per step (and golang-migrate Goto currently returns a single entry). Prefer printing before/after versions, a driver-provided count, or deriving steps from Status() before/after.
| } | ||
|
|
||
| // Enumerate pending migrations (versions in source that exceed current). | ||
| pending, _ := listPendingVersions(req.Source.Dir, version, atNil) |
There was a problem hiding this comment.
Status() discards the error returned by listPendingVersions. If reading the migration source fails, Status will return an incomplete/incorrect Pending list instead of surfacing the failure. Consider returning an error when listPendingVersions fails.
| pending, _ := listPendingVersions(req.Source.Dir, version, atNil) | |
| pending, err := listPendingVersions(req.Source.Dir, version, atNil) | |
| if err != nil { | |
| return interfaces.MigrationStatus{}, fmt.Errorf("golang-migrate status: list pending versions: %w", err) | |
| } |
| // HistoryTable is the migration history table name (driver default if empty). | ||
| HistoryTable string `json:"history_table"` | ||
| // Timeout for migration operations. Default: 5m. |
There was a problem hiding this comment.
HistoryTable is exposed in module config, but it is never used when constructing requests or initializing drivers, so the option has no effect. Either wire it through to the underlying drivers (if supported) or remove it to avoid misleading users.
| return interfaces.MigrationResult{ | ||
| Applied: []string{target}, | ||
| DurationMs: time.Since(start).Milliseconds(), | ||
| }, nil |
There was a problem hiding this comment.
Goto() always returns Applied = []string{target}, even when m.Migrate returns ErrNoChange or when multiple versions were traversed. This causes callers to misreport what happened. Consider capturing the version before/after and returning an accurate Applied list (or empty slice on no-op).
| } | ||
|
|
||
| // Enumerate pending migrations (versions in source that exceed current). | ||
| pending, _ := listPendingVersions(req.Source.Dir, version, atNil) |
There was a problem hiding this comment.
Status() discards the error returned by listPendingVersions. If reading the migration source fails, Status will return an incomplete/incorrect Pending list instead of surfacing the failure. Consider returning an error when listPendingVersions fails.
| pending, _ := listPendingVersions(req.Source.Dir, version, atNil) | |
| pending, err := listPendingVersions(req.Source.Dir, version, atNil) | |
| if err != nil { | |
| return interfaces.MigrationStatus{}, fmt.Errorf("golang-migrate status: list pending versions: %w", err) | |
| } |
| // Adapter wraps any MigrationDriver and adds optional pre/post hooks. | ||
| type Adapter struct { | ||
| inner Driver | ||
| } |
There was a problem hiding this comment.
The Adapter type comment claims it "adds optional pre/post hooks", but the implementation currently only delegates directly to the inner driver and has no hook mechanism. Consider either implementing hook support or adjusting the comment to match the current behavior.
| v, err = s.Next(v) | ||
| } | ||
| return pending, nil | ||
| } |
There was a problem hiding this comment.
listPendingVersions() ignores the terminal error from Next()/First() and always returns a nil error. This can hide source iteration failures and silently produce incomplete Pending output. Consider returning nil only for the expected end-of-list condition and propagating other errors.
Summary
workflow-plugin-migrationsplugin repo targetinggithub.com/GoCodeAlone/workflow v0.18.2What's included
Module types
database.migrations— holds driver config (driver, source_dir, dsn_env, timeout); dispatches Up/Down/Status/Goto viaInvokeMethoddatabase.migration_driver— declares a named driver (golang-migrate | goose | atlas)Step types
step.migrate_up— apply all pending migrationsstep.migrate_down— roll back N migrations (default 1)step.migrate_status— return current version + pending liststep.migrate_to— migrate to a specific versionDrivers
internal/golangmigrate— wrapsgolang-migrate/migrate/v4with pgx/v5 backendinternal/goose— wrapspressly/goose/v3with pgx stdlib backendinternal/atlasplugin— stub for Task 27 (Atlas driver)Infrastructure
pkg/driver— MigrationDriver adapter + in-process registrypkg/testharness— embedded-postgres auto-fallback harness (no Docker required locally)pkg/conformance— exportedSuitewith 6 behavioural test cases + embedded corpus SQLpkg/cli— shared Cobra root (up/down/status/goto) for bothwfctl migrate *andworkflow-migrateBinaries
cmd/workflow-plugin-migrations— main plugin binary (golang-migrate + goose + wfctl CLI)cmd/workflow-plugin-atlas-migrate— atlas-only binarycmd/workflow-migrate— standalone pre-deploy runnerCI/Release
.github/workflows/ci.yml— postgres service container for integration tests.github/workflows/release.yml— goreleaser v2 with 3 archives + SBOMs + checksumsTest plan
go build ./...— cleango vet ./...— cleango test ./... -short -race— all pass (15+ unit tests)go test ./...(integration) — runs in CI via postgres service; skips locally when no postgres availablego.mod— noreplacedirectives; uses taggedgithub.com/GoCodeAlone/workflow v0.18.2🤖 Generated with Claude Code