Skip to content
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

Don't return an error when migrations are up to date #599

Closed
oliverpool opened this issue Sep 19, 2023 · 9 comments
Closed

Don't return an error when migrations are up to date #599

oliverpool opened this issue Sep 19, 2023 · 9 comments

Comments

@oliverpool
Copy link

oliverpool commented Sep 19, 2023

First of all, thank your for this great project!

Since #539, the goose.CollectMigrations function returns an error when no migration has been found (as requested in #336) or when the migrations are up to date (I just got bitten by this).

Usecase:

Upon startup, my program checks if any migration is pending and applies them if needed (it make a backup of the DB before migrating, hence the need to know if a migration is pending).
Until now if my program already had the latest migration, it would simply return nil.
With the change made here, my program fails with no migration files found.

// MissingMigration:
//
//   - retrieves the current version for this DB (create and initialize the DB version table if it doesn't exist)
//   - collects the migrations inside the provided FS
//   - returns a func to run the missing migrations (AlreadyUpToDate if no migration is missing)
func missingMigrations(dialect string, db *sql.DB, fs fs.FS) (func() error, error) {
	goose.SetBaseFS(fs)
	if err := goose.SetDialect(dialect); err != nil {
		return nil, err
	}
	version, err := goose.EnsureDBVersion(db)
	if err != nil {
		return nil, err
	}

	m, err := goose.CollectMigrations(".", version, goose.MaxVersion) // this now fails
	if err != nil {
		return nil, err
	}
	if len(m) == 0 {
		return AlreadyUpToDate, nil
	}

	return func() error {
		return goose.Up(db, ".")
	}, nil
}

var AlreadyUpToDate = func() error { return nil }

Suggestion:

  1. Change the logic of collectMigrationsFS to not return an error if an "already run" migration was found.
  2. Or provide a function as the one above to ease the use of goose as a library

I would be willing to draft a PR if you think that this is a usecase worth supporting.

@oliverpool oliverpool changed the title Don't return on error when migrations are up to date Don't return an error when migrations are up to date Sep 19, 2023
@mfridman
Copy link
Collaborator

mfridman commented Oct 3, 2023

I seem to have missed these notifications. Just got back from GopherCon and working through a backlog of issues / PRs.

Thanks for filing a few issues and providing feedback. I plan to address these in the next week, even if just to jot down / comment some thoughts.

@Zelinzky
Copy link

I stumbled upon this while trying something similar, my program does not check and then updates, it only checks, if the migrations has not been applyed it closes. (this because i want migrations to be run appart from the app, but to not run if they are not there)

I have this function in one of my packages that is doing the verification. It just adds an error.Is check, I think there can be a function on the package level that does something similar and allows to check if the migrations and the db are in sync.

func IsMigUpToDate(db *sqlx.DB, fs fs.FS, dir string) (bool, error) {
	goose.SetBaseFS(fs)
	err := goose.SetDialect("postgres")
	if err != nil {
		return false, fmt.Errorf("check migration: %w", err)
	}
	version, err := goose.GetDBVersion(db.DB)
	if err != nil {
		return false, err
	}
	_, err = goose.CollectMigrations(dir, version, goose.MaxVersion)
	if errors.Is(err, goose.ErrNoMigrationFiles) {
		return true, nil
	}
	if err != nil {
		return false, fmt.Errorf("check migration: %w", err)
	}
	return false, nil

@oliverpool hope this helps.

@mfridman
Copy link
Collaborator

mfridman commented Dec 15, 2023

I need to think this through a bit, but iirc my first reaction was .. this works if all migrations are sequential, but would technically not work if the out-of-order (allow missing) migrations feature is enabled. Since the latest applied max DB version could be higher than incoming unapplied out-of-order migration(s).

I suppose there could be a short-circuit that "if out of order is disabled, and no new migrations are detected even if there are no new migration files" then return no error).

Tbh, I didn't realize folks were using the API this way. Sorry about that, sounds like a regression on our part!

@oliverpool
Copy link
Author

@Zelinzky, yes, this is how I solved it:

https://codeberg.org/pfad.fr/fahrtkosten/src/commit/65a5679d69afa2fde8e8ed3aeed3e1d2a9987a2f/internal/migrations/migration.go#L27

(I should use errors.Is instead of == as you propose)

@mfridman
Copy link
Collaborator

mfridman commented Dec 15, 2023

I'm curious, why do you need this logic?

The internals of goose handle this case, for example, when you call Up it'll noop if there's no pending migrations.

Afaics you want to have custom logic

  • Check if pending migrations
  • Do some custom logic in you app
  • Maybe apply migrations or error, etc

@oliverpool
Copy link
Author

oliverpool commented Dec 15, 2023

Exactly, if there are pending migrations, I make a backup of the database before running the migrations.

https://codeberg.org/pfad.fr/fahrtkosten/src/commit/65a5679d69afa2fde8e8ed3aeed3e1d2a9987a2f/cmd/server/main.go#L338-L349

@Zelinzky
Copy link

In my case I handle the migrations outside of the server/app initialization. I just want my pod to exit verbosely if there are migrations to be applied.

@mfridman
Copy link
Collaborator

Okay, that's a fair point. I'll put some time to think what can be added and how this can be resolved to minimize impact.

@mfridman
Copy link
Collaborator

We recently added 2 methods to the goose Provider that enable you to check for "pending" migrations and also get the latest current/target versions.

// HasPending returns true if there are pending migrations to apply, otherwise, it returns false. If
// out-of-order migrations are disabled, yet some are detected, this method returns an error.
//
// Note, this method will not use a SessionLocker if one is configured. This allows callers to check
// for pending migrations without blocking or being blocked by other operations.

func (p *Provider) HasPending(ctx context.Context) (bool, error)

// GetVersions returns the max database version and the target version to migrate to.
//
// Note, this method will not use a SessionLocker if one is configured. This allows callers to check
// for versions without blocking or being blocked by other operations.

func (p *Provider) GetVersions(ctx context.Context) (current, target int64, err error)

I believe these 2 methods would accomplish what's mentioned in this issue.

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

No branches or pull requests

3 participants