Update aws go version to v2#1384
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 <dgarcia@infoblox.com> Co-authored-by: Ying Chen <108433798+ychen-bloxer@users.noreply.github.com> Co-authored-by: Supriya Gowda <105773269+Gowdasupriya@users.noreply.github.com> * register pgx in main cli, not internal (#30) * register pgx in main cli, not internal * go fmt --------- Co-authored-by: Sujay Kumar Suman <ssuman2@infoblox.com> Co-authored-by: Ying Chen <108433798+ychen-bloxer@users.noreply.github.com> Co-authored-by: Supriya Gowda <105773269+Gowdasupriya@users.noreply.github.com>
…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
# Conflicts: # go.mod # go.sum
There was a problem hiding this comment.
Pull request overview
This PR updates the project to Go 1.25 and migrates the S3 source driver to AWS SDK for Go v2, while also introducing a number of additional changes (CLI configuration via pflag/viper, dirty DB state handling with a migration cache directory, Docker packaging changes, and CI/test adjustments).
Changes:
- Migrate
source/aws_s3from AWS SDK v1 to AWS SDK v2 and add paginated listing support. - Add “dirty-state handling” flow in
migrate.Migratewith file-based last-success tracking, plus associated tests. - Rework CLI/build/CI/container packaging (pflag/viper config, logrus, new
install-tocommand, updated Dockerfiles/Makefile/workflows).
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| source/aws_s3/s3.go | Switch S3 driver implementation to AWS SDK v2 and add pagination for object listing. |
| source/aws_s3/s3_test.go | Update S3 fake client/tests for AWS SDK v2 interfaces/types. |
| README.md | Update supported Go versions badge. |
| migrate.go | Add dirty-state handling configuration and execution flow; add file copy/cleanup helpers. |
| migrate_test.go | Add tests for dirty-state handling helpers and config parsing. |
| Makefile | Change versioning and redefine build target to build a Docker image. |
| Jenkinsfile | Add Jenkins pipeline definition for building images (and currently commented unit tests). |
| internal/cli/main.go | Rework CLI argument/config handling to use pflag+viper; add install-to; add dirty-handling flags. |
| internal/cli/log.go | Switch CLI logging backend to logrus. |
| internal/cli/commands_install_to.go | Add install-to command implementation (copy running binary). |
| internal/cli/commands_install_to_test.go | Add unit/integration tests for install-to. |
| go.mod | Bump Go version to 1.25 and update dependencies (incl. AWS SDK v2, viper/pflag, logrus, multierror). |
| go.sum | Dependency checksum updates corresponding to go.mod changes. |
| Dockerfile.github-actions | Change base image to alpine:latest. |
| Dockerfile | Change build context path and switch runtime image to distroless; copy config into image. |
| docker-deploy.sh | Add --pull to docker build. |
| database/sqlserver/sqlserver_test.go | Unconditionally skip SQL Server tests. |
| cmd/migrate/main.go | Add viper/pflag + config loading setup and hotload driver registration. |
| cmd/migrate/config/defaults.yaml | Add default config file for the containerized CLI. |
| cmd/migrate/config.go | Define pflag flags for config-driven CLI. |
| cmd/migrate/.gitignore | Ignore built migrate binary under cmd/migrate. |
| cli/main.go | Keep deprecated wrapper main package. |
| .github/workflows/ci.yaml | Drop Go 1.24 from matrix, disable lint job, and adjust test DB tag set. |
Comments suppressed due to low confidence (1)
cli/main.go:10
internal/cli.Mainnow relies on pflag/viper being initialized (Parse, BindPFlags, config/env setup) before it runs. This deprecated wrapper binary callscli.Main(Version)without that initialization, so flags/config won’t be read and command parsing may break for users still invoking the legacyclientrypoint. Consider keeping the old flag-parsing path here or calling the same initialization used incmd/migrate/main.gobefore invokinginternal/cli.Main.
// Deprecated, please use cmd/migrate
func main() {
cli.Main(Version)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,4 +1,4 @@ | |||
| FROM alpine:3.19 | |||
| FROM alpine:latest | |||
There was a problem hiding this comment.
Using alpine:latest makes builds non-reproducible and can unexpectedly pull in breaking changes or vulnerable images over time. Pin to a specific Alpine version (and ideally a digest) to keep CI builds deterministic.
| FROM alpine:latest | |
| FROM alpine:3.20 |
| databasePtr := flag.String("database", "", "") | ||
| sourcePtr := flag.String("source", "", "") | ||
| help := viper.GetBool("help") | ||
| version = viper.GetString("version") |
There was a problem hiding this comment.
version is overwritten from viper config (viper.GetString("version")), but --version historically behaved as a boolean flag (print version and exit). With the current logic, --version won’t work as intended and can even require a string argument. Consider restoring a boolean --version flag (or a separate --print-version) and don’t overwrite the passed-in version string that should be printed.
| version = viper.GetString("version") | |
| printVersion := viper.GetBool("version") | |
| _ = printVersion |
|
|
||
| // show cli version | ||
| if *versionPtr { | ||
| if version == "" { |
There was a problem hiding this comment.
The version printing condition is inverted: if version == "" { fmt.Fprintln(os.Stderr, version); os.Exit(0) } prints an empty string and exits, while the non-empty version never prints. This should check the version flag (boolean) and print the actual version value.
| if version == "" { | |
| if version != "" { |
| var ( | ||
| // define flag overrides | ||
| flagHelp = pflag.Bool("help", false, "Print usage") | ||
| flagVersion = pflag.String("version", Version, "Print version") |
There was a problem hiding this comment.
--version is declared as a string flag with default Version, which changes the CLI contract (it now expects an argument) and interacts badly with viper’s GetString("version") usage. To preserve standard CLI behavior, this should be a boolean flag (print version and exit), while the actual version value should remain the Version variable.
| flagVersion = pflag.String("version", Version, "Print version") | |
| flagVersion = pflag.Bool("version", false, "Print version") |
| } | ||
| scheme := "file" | ||
| if uri.Scheme != "file" && uri.Scheme != "" { | ||
| return "", "", fmt.Errorf("unsupported scheme: %s", scheme) |
There was a problem hiding this comment.
The unsupported-scheme error reports scheme (always "file") instead of the actual unsupported uri.Scheme, which makes debugging harder. Use the parsed scheme in the error message so callers can see what was rejected.
| return "", "", fmt.Errorf("unsupported scheme: %s", scheme) | |
| return "", "", fmt.Errorf("unsupported scheme: %s", uri.Scheme) |
| -help Print usage | ||
| --source Location of the migrations (driver://url) | ||
| --path Shorthand for -source=file://path | ||
| --database Run migrations against this database (driver://url) |
There was a problem hiding this comment.
The help text documents --database but the code reads database.dsn (and constructs a DSN from database.*). Either update the usage text to the actual flag name (--database.dsn) or add a compatibility --database flag that maps to the same config key.
| --database Run migrations against this database (driver://url) | |
| --database.dsn Run migrations against this database (driver://url) |
| version: false | ||
| verbose: true | ||
| prefetch: 10 | ||
| lockTimeout: 15 |
There was a problem hiding this comment.
The defaults file keys/types don’t match what the CLI reads: the code uses viper.GetString("version") (string) and viper.GetInt("lock-timeout"), but this file sets version: false (bool) and lockTimeout (camelCase). As written, these defaults won’t apply—align the keys and keep version consistently typed.
| version: false | |
| verbose: true | |
| prefetch: 10 | |
| lockTimeout: 15 | |
| version: "" | |
| verbose: true | |
| prefetch: 10 | |
| lock-timeout: 15 |
| if m.IsDirtyHandlingEnabled() { | ||
| // this condition is required if the first migration fails | ||
| if lastCleanMigrationApplied == 0 { | ||
| lastCleanMigrationApplied = migr.TargetVersion | ||
| } |
There was a problem hiding this comment.
When a migration Run fails, the code sets lastCleanMigrationApplied = migr.TargetVersion (when still 0) and persists it as “last successful”. For an up migration, TargetVersion is the new version that failed, so later dirty-state recovery can incorrectly mark an unapplied version as clean. Track the last clean DB version before setting dirty (e.g., initialize from curVersion and update only after successful SetVersion(..., false)).
| fileName := file.Name() | ||
| migration, err := source.Parse(fileName) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
cleanupFiles calls source.Parse on every directory entry and returns an error for non-migration files. If destPath contains unrelated files (or a marker like lastSuccessfulMigration), cleanup will fail and block successful migrations. Filter using source.Regex (and/or explicitly skip lastSuccessfulMigrationFile) before parsing/removing.
| [](https://coveralls.io/github/golang-migrate/migrate?branch=master) | ||
| [](https://packagecloud.io/golang-migrate/migrate?filter=debs) | ||
| [](https://hub.docker.com/r/migrate/migrate/) | ||
|  | ||
|  | ||
| [](https://github.com/golang-migrate/migrate/releases) |
There was a problem hiding this comment.
The PR title suggests only updating the AWS Go SDK to v2, but this diff also introduces substantial new behavior (dirty-state handling, CLI config via viper/pflag, Docker/CI changes, new install-to command). Consider updating the PR title/description to reflect the broader scope, or splitting into smaller PRs to reduce review and rollback risk.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
cli/main.go:10
cli/main.go(deprecated entrypoint) callsinternal/cli.Maindirectly, but the updated CLI now depends on pflag/Viper initialization done incmd/migrate/main.go(binding flags, env, and optional config). Without similar initialization here, the deprecated binary will not parse flags/config correctly. Either keep the legacy CLI behavior or add the minimal init/setup here too.
import (
"github.com/golang-migrate/migrate/v4/internal/cli"
)
// Deprecated, please use cmd/migrate
func main() {
cli.Main(Version)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| scheme := "file" | ||
| if uri.Scheme != "file" && uri.Scheme != "" { | ||
| return "", "", fmt.Errorf("unsupported scheme: %s", scheme) |
There was a problem hiding this comment.
In parsePath(), the returned error message always reports the hardcoded scheme value ("file") rather than the actual unsupported uri.Scheme. This will mislead users (e.g., s3:///... will error as "unsupported scheme: file"). Use uri.Scheme in the error and consider keeping the returned scheme consistent with the validated value.
| return "", "", fmt.Errorf("unsupported scheme: %s", scheme) | |
| return "", "", fmt.Errorf("unsupported scheme: %s", uri.Scheme) |
| if err := m.databaseDrv.Run(migr.BufferedBody); err != nil { | ||
| if m.IsDirtyHandlingEnabled() { | ||
| // this condition is required if the first migration fails | ||
| if lastCleanMigrationApplied == 0 { | ||
| lastCleanMigrationApplied = migr.TargetVersion | ||
| } | ||
| if e := m.handleMigrationFailure(lastCleanMigrationApplied); e != nil { | ||
| return multierror.Append(err, e) | ||
| } |
There was a problem hiding this comment.
lastCleanMigrationApplied is initialized to 0 and, on the first migration failure, is set to migr.TargetVersion. That value represents the post-migration version, not the last successfully applied clean version, so the written lastSuccessfulMigration can be incorrect. Track the last clean version explicitly (e.g., initialize from m.databaseDrv.Version() before the loop and update it only after a successful SetVersion(..., false)), and avoid using 0 as a sentinel since version 0 can be valid.
| // show cli version | ||
| if *versionPtr { | ||
| if version == "" { | ||
| fmt.Fprintln(os.Stderr, version) | ||
| os.Exit(0) | ||
| } |
There was a problem hiding this comment.
The version printing logic is inverted: if version == "" { fmt.Fprintln(...); os.Exit(0) } prints an empty string and exits when the version is empty, rather than when the user requested --version. This makes migrate --version ineffective and can cause silent exits if config coercion yields an empty string. Gate this on a dedicated boolean flag and print the actual build version string.
| @@ -3,7 +3,7 @@ ARG VERSION | |||
|
|
|||
| RUN apk add --no-cache git gcc musl-dev make | |||
There was a problem hiding this comment.
The final image copies /etc/ssl/certs/ from the builder stage, but the builder image doesn’t install ca-certificates, so this directory may be empty and TLS connections can fail at runtime. Install ca-certificates in the builder (or add a separate certs stage) before copying, or use a distroless base variant that already includes certificates.
| RUN apk add --no-cache git gcc musl-dev make | |
| RUN apk add --no-cache git gcc musl-dev make ca-certificates |
| m := &Migrate{} | ||
| err := m.WithDirtyStateConfig(tt.srcPath, tt.destPath, tt.isDirty) | ||
| if (err != nil) != tt.wantErr { | ||
| t.Errorf("error = %v, wantErr %v", err, tt.wantErr) | ||
| return | ||
| } | ||
| if !tt.wantErr && m.dirtyStateConf == tt.wantConf { | ||
| t.Errorf("dirtyStateConf = %v, want %v", m.dirtyStateConf, tt.wantConf) | ||
| } | ||
| }) |
There was a problem hiding this comment.
In TestWithDirtyStateConfig, the assertion compares pointers (m.dirtyStateConf == tt.wantConf) and the condition is inverted, so the test doesn’t actually validate the produced config values. Use a field-by-field comparison (e.g., assert.Equal / reflect.DeepEqual) and fail when the config differs from wantConf.
| if err = m.handleDirtyState(); err != nil { | ||
| if strings.Contains(err.Error(), test.err.Error()) { | ||
| t.Logf("expected error %v, got %v", test.err, err) | ||
| if !test.setupFailure { | ||
| if err = os.Remove(lastSuccessfulMigrationPath); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } | ||
| return | ||
| } else { | ||
| t.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
In the error-check branch, strings.Contains(err.Error(), test.err.Error()) will panic if test.err is nil (several test cases set err: nil). If handleDirtyState() unexpectedly returns an error in those cases, the test will crash rather than fail with a useful message. Guard against test.err == nil and use explicit expected-error assertions.
| if err = m.handleDirtyState(); err != nil { | |
| if strings.Contains(err.Error(), test.err.Error()) { | |
| t.Logf("expected error %v, got %v", test.err, err) | |
| if !test.setupFailure { | |
| if err = os.Remove(lastSuccessfulMigrationPath); err != nil { | |
| t.Fatal(err) | |
| } | |
| } | |
| return | |
| } else { | |
| t.Fatal(err) | |
| } | |
| } | |
| err = m.handleDirtyState() | |
| if test.err != nil { | |
| if err == nil { | |
| t.Fatalf("expected error %v, got nil", test.err) | |
| } | |
| if !strings.Contains(err.Error(), test.err.Error()) { | |
| t.Fatalf("expected error containing %q, got %v", test.err.Error(), err) | |
| } | |
| t.Logf("expected error %v, got %v", test.err, err) | |
| if !test.setupFailure { | |
| if err = os.Remove(lastSuccessfulMigrationPath); err != nil { | |
| t.Fatal(err) | |
| } | |
| } | |
| return | |
| } | |
| if err != nil { | |
| t.Fatalf("expected no error, got %v", err) | |
| } |
No description provided.