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

Go Migrations don't work #582

Closed
paulrozhkin opened this issue Aug 10, 2023 · 8 comments · Fixed by #588
Closed

Go Migrations don't work #582

paulrozhkin opened this issue Aug 10, 2023 · 8 comments · Fixed by #588
Labels
Milestone

Comments

@paulrozhkin
Copy link

Goose v3.14.0

I have one .go migration file that I register with via AddMigrationContext. After registering and trying to migrate, I get an ErrNoMigrationFiles error.
I think the error is in collectMigrationsFS on line 297. Maybe there should be a !ok check here?

@mfridman
Copy link
Collaborator

There's a check to make sure registered Go migrations exist on the filesystem (os or embedded).

I realize the example doesn't highlight the important part, which is:

  1. You need to init() that AddMigrationContext function
  2. Blank import that package as part of your build

If you point me at a repo or an example, I could help you debug it. But, I think it's probably best I sat down and provided a blog/sample repo for how to do this step-by-step.

@edulop91
Copy link

hi @mfridman ! We're also running into this issue when we upgraded to 3.15. Things worked fine before.
Confirming we do both steps you suggest:

You need to init() that AddMigrationContext function
Blank import that package as part of your build

Since we have no fsGoMigrations, it seems like this block will never add any registeredGoMigrations

	// Go migrations registered via goose.AddMigration().
	for _, migration := range registeredGoMigrations {
		v, err := NumericComponent(migration.Source)
		if err != nil {
			return nil, fmt.Errorf("could not parse go migration file %q: %w", migration.Source, err)
		}
		if !versionFilter(v, current, target) {
			continue
		}
		if _, ok := fsGoMigrations[v]; ok {
			migrations = append(migrations, migration)
		}
	}

@tom5760
Copy link

tom5760 commented Aug 15, 2023

Ran into the same issue. For us, we are including all of our SQL files in an embed.FS. A work-around we found was to also add the *.go to our embedded FS.

// Needed to add *.go below
//go:embed *.sql *.go
var schemaFS embed.FS

// ...

goose.SetBaseFS(schemaFS)

@mfridman
Copy link
Collaborator

Indeed, there was an assumption the .go files would live alongside the .sql files and could be read from either the os filesystem or an embedded filesystem.

But, that appears to be a wrong assumption. Sorry about that, I need to think about how to resolve this so it doesn't break folks from #553.

Two immediate solutions come to mind

@mfridman
Copy link
Collaborator

@edulop91 @paulrozhkin is it safe to assume you're also embedding migrations similar to @tom5760 ?

@paulrozhkin
Copy link
Author

@mfridman Now I made downgrade to previous version 3.13.4.
I expected that AddMigration in init() to not require embed resources.

@mfridman
Copy link
Collaborator

mfridman commented Aug 23, 2023

Alright, I think there's a path forward here.

  1. If Go migrations have been registered, but there are no corresponding .go files in the filesystem, add them to the migrations slice (old behaviour). This should resolve the current issue
  1. If Go migrations have been registered, and there are .go files in the filesystem, only include those in the migrations slices. This should maintain the desired behaviour from

Now, there's one property I'd like to maintain, if .go files (that have a version number) exist in the filesystem that have not been registered raise an error.

Return an error if the given sources contain a versioned Go migration that has not been
registered. This is a sanity check to ensure users didn't accidentally create a valid
looking Go migration file and forget to register it.

This is almost always a user error.

@mfridman
Copy link
Collaborator

mfridman commented Aug 26, 2023

Put up a PR to resolve this issue. #588

If anyone would like to provide early feedback (or even better, take this branch for a spin) that'd be great.

TL;DR #588 should bring back the original behaviour, while also providing a way to filter migrations if they exist in the filesystem.

EDIT: also added a bunch of tests to capture all 3 scenarios mentioned in #582 (comment)

mfridman added a commit that referenced this issue Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants