Filter registered migrations by fs dirpath

[rafalneves]

Closes #552.

Filters registered migration by correspondent fs file in the target dir path. This way, the same binary, can run goose multiple times, over different migration directories. Otherwise, all go migrations will run in the first goose run.

This modification shouldn't be a breaking change, at least not semantically. This was the behavior that I was expecting.
This way we are sure that only migrations in the specified directory are applied on a given run.

[rafalneves]

Hi @mfridman, can you take a look at this, please?

[mfridman]

If I understand correctly, before the code was including ALL go migrations from the registered global, but with this change we're filtering that global down to only the .go files from the filesystem. Essentially, this...

for _, migration := range registeredGoMigrations {
        //
	if _, ok := fsGoMigrations[v]; ok {
		migrations = append(migrations, migration)
	}
}

At first, this seems like a breaking change, but after looking at this a bit closer I don't think it is. The code previously assumed all .go migrations would be in the same folder and thus registered the same way, so that actually doesn't change 🤔

[rafalneves]

If I understand correctly, before the code was including ALL go migrations from the registered global, but with this change we're filtering that global down to only the .go files from the filesystem.

Yeap, that's correct.

[mfridman]

Okay, this seems reasonable to me. Fwiw, this actually makes the implementation match the /v4 branch:

goose/provider.go

Lines 104 to 108 in 3d10b39

	if goMigration, ok := registered[src.Version]; ok {
	m = goMigration
	} else {
	unregistered = append(unregistered, currentBase)
	}

... where we loop over the "sources" (filesystem), and ensure everything in the filesystem is registered in the global map.

In BOTH cases if a versioned filesystem .go migration isn't registered, it'll fail (this is the main thing I was looking for). This is intended to be a guard for users who added a "version"-looking migration file but didn't register it with an init. In the /v4 we have this comment to make this clear and a corresponding error message.

// 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]

lgtm.

[rafalneves]

Perfect, thanks for the review 🙌 . Please merge when ready.

[astromechza]

Unfortunately this was a breaking change for my app. I've been registering migrations that don't exist on the filesystem at all and are added as pure go functions. They now get excluded completely.

At least this got caught by the ErrNoMigrationFiles so we can detect and fix it.

I'll look at moving them to file-based migrations but I would prefer the single-binary approach.

[mfridman]

Ouch, sorry about that @astromechza. To clarify, were you using something like AddNamedMigration (or its equivalents) and adding a filename directly, but one that didn't exist on the filesystem?

I typically use Go's embed to include the files in the binary itself:

//go:embed *.sql
var EmbedMigrations embed.FS

And then goose.SetBaseFS(EmbedMigrations) to include them at the time of building the binary.

However, I could see a world where there is a "pure register function" that simply takes a version int64 and a function. We can them resolve whether it's a duplicate or not, but avoid touching the filesystem in its entirety.

[astromechza]

Yup, exactly, we were calling AddNamedMigration :), not big problem though, I've moved over to the embed FS since that's the one that's documented.

I think the docs do mention using AddMigration directly, and may need to be updated.

[mfridman]

Thanks for your report @astromechza, going to track this issue in #582

I'd like to bring back the ability to register pure go functions without forcing users to embed the source.