-
Notifications
You must be signed in to change notification settings - Fork 1.5k
WIP: Store migrates in db #1318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Store migrates in db #1318
Conversation
- Add support for viper
Enable configuration
- Add support for dsn and individual parameters
Configuration support for database string
Merge latest changes from opensource migrate
* Update alpine and golang versions * Refresh go modules
* 'master' of github.com:golang-migrate/migrate: (418 commits) Run gofmt on internal build dir go mod tidy Resolves golang-migrate#647 - Fixes typos in Mongo advisory locking parameters (golang-migrate#648) Bump version of autorest/adal Set syntax highlighting for pkger example Add pkger to README change github auth to use oauth token instead of basic. Use the recommended v4 in mysql README go mod tidy fix test Delete all rows Use ParseBool Support for AWS Keyspaces Update gosnowflake from v1.4.3 to v1.6.3 Update docker client usage with breaking change Update dktest to v0.3.7 to fix security warnings revert binary file location change in docker image fix source/file driver Update build constraints Update golangci-lint config ...
* upgrade alpine 3.13 -> 3.18 * add --pull to docker build
* adding pgx as the supported hotload base driver * updating go.mod file
* pgx migrate test (#29) * add hotload support (#15) * import fsync * Upgrade alpine 3.13 -> 3.18 (#21) * upgrade alpine 3.13 -> 3.18 * add --pull to docker build * adding pgx as the supported hotload base driver (#26) * adding pgx as the supported hotload base driver * updating go.mod file * removing the pgx register in migrate (#27) * register hotload with pgx driver --------- Co-authored-by: Daniel Garcia <[email protected]> Co-authored-by: Ying Chen <[email protected]> Co-authored-by: Supriya Gowda <[email protected]> * register pgx in main cli, not internal (#30) * register pgx in main cli, not internal * go fmt --------- Co-authored-by: Sujay Kumar Suman <[email protected]> Co-authored-by: Ying Chen <[email protected]> Co-authored-by: Supriya Gowda <[email protected]>
* First pass at downmigrate * First pass at downmigrate * downmigrate changes + UTs + cleanups * address comments
…own feature (#37) * address comments * address comments + new UTs * address comments * remove build files
* Update go version and dependencies * Update go versions in ci
- Add new 'install-to DIR' subcommand that copies the running binary to a specified directory - Uses atomic operations: copy to temp file, then rename for atomicity - Preserves executable permissions (755) from original binary - Handles errors gracefully with proper exit codes and cleanup - Validates destination directory exists and is actually a directory - Integrated with existing help system and command structure Implementation details: - New file: internal/cli/commands_install_to.go - core functionality - New file: internal/cli/commands_install_to_test.go - comprehensive test suite - Modified: internal/cli/main.go - CLI integration and command parsing Testing: - 7 test functions covering unit and integration testing - Tests atomic operations, error handling, permission preservation - Tests CLI interface, help system, and edge cases - All tests passing This feature enables easy binary deployment and self-updating workflows while maintaining compatibility with upstream migrate project structure.
feat: add install-to subcommand for binary self-installation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This is a WIP pull request that adds the ability to store migration scripts directly in the database instead of relying on external file storage for dirty state handling. This enables better migration management in environments where shared file storage is not available.
Key changes include:
- Introduction of database-based migration storage capabilities through a new
MigrationStorageDriver
interface - Enhanced dirty state handling with support for both file-based and database-based approaches
- Addition of an "install-to" command for copying the migrate binary to specified directories
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
migrate.go | Core migration logic enhanced with database storage sync, dirty state handling, and file cleanup operations |
migrate_test.go | Comprehensive test coverage for new dirty state handling, file operations, and configuration features |
database/storage.go | New interface defining database-based migration storage capabilities |
database/postgres/storage.go | PostgreSQL implementation of migration storage with enhanced schema table support |
database_source.go | Source driver implementation for reading migrations from database storage |
internal/cli/main.go | CLI enhancements with viper configuration, database driver support, and new install-to command |
cmd/migrate/main.go | Updated main entry point with hotload driver registration and configuration setup |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
if version == "" { | ||
fmt.Fprintln(os.Stderr, version) | ||
os.Exit(0) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is inverted - if version is empty, it prints the empty version and exits. This should check if a version flag was set to true or if version is non-empty to print and exit.
Copilot uses AI. Check for mistakes.
} | ||
scheme := "file" | ||
if uri.Scheme != "file" && uri.Scheme != "" { | ||
return "", "", fmt.Errorf("unsupported scheme: %s", scheme) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message uses variable scheme
but should use uri.Scheme
since scheme
is declared as a constant string 'file' on line 227.
return "", "", fmt.Errorf("unsupported scheme: %s", scheme) | |
return "", "", fmt.Errorf("unsupported scheme: %s", uri.Scheme) |
Copilot uses AI. Check for mistakes.
m, _ := setupMigrateInstance(tempDir) | ||
|
||
tests := []struct { | ||
lastSuccessFulVersion int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The field name contains a typo - 'lastSuccessFulVersion' should be 'lastSuccessfulVersion' to match the spelling used elsewhere in the codebase.
Copilot uses AI. Check for mistakes.
if !tt.wantErr && m.dirtyStateConf == tt.wantConf { | ||
t.Errorf("dirtyStateConf = %v, want %v", m.dirtyStateConf, tt.wantConf) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comparison logic is incorrect. It should use !=
instead of ==
to properly compare if the actual configuration doesn't match the expected configuration when no error is expected.
Copilot uses AI. Check for mistakes.
// this condition is required if the first migration fails | ||
if lastCleanMigrationApplied == 0 { | ||
lastCleanMigrationApplied = migr.TargetVersion | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is problematic - if the first migration fails, lastCleanMigrationApplied
is set to the current failing migration's target version, but no migration has actually been successfully applied yet. This should use the previous version or handle the zero case differently.
// this condition is required if the first migration fails | |
if lastCleanMigrationApplied == 0 { | |
lastCleanMigrationApplied = migr.TargetVersion | |
} | |
// Do not update lastCleanMigrationApplied on failure; it should remain zero if no migration has succeeded |
Copilot uses AI. Check for mistakes.
if err.Error() == "file does not exist" { // Handle os.ErrNotExist | ||
break | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing error strings is fragile and unreliable. Use errors.Is(err, os.ErrNotExist)
or check for the specific error type instead of string comparison.
Copilot uses AI. Check for mistakes.
No description provided.