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

WIP: Add support for Go embedded file system #346

Closed

Conversation

Enrico204
Copy link
Contributor

@Enrico204 Enrico204 commented Dec 3, 2022

This commit adds the support for fs.FS filesystem. The default value is an implementation that forwards Open() to os.Open() for opening filesystem paths (like before this change).

This is a rebase/rework of the previous branch by @bouk

WIP:

  • Add instructions in README.md
  • Add examples

@Enrico204 Enrico204 changed the title Add support for Go embedded file system WIP: Add support for Go embedded file system Dec 3, 2022
@gnuletik
Copy link
Contributor

@amacneil Does that PR looks good to you? Is there anything I can do to help get it merged? Thanks!

@Enrico204
Copy link
Contributor Author

Enrico204 commented Jan 17, 2023

@amacneil Does that PR looks good to you? Is there anything I can do to help get it merged? Thanks!

There are two items to check in the issue description: instructions in the README, and examples. At least those items should be done before considering this PR worth to merge IMHO.

@gnuletik
Copy link
Contributor

Thanks for the feedback @Enrico204 !
I tested the PR locally and it works like a charm.

However, I cannot add examples and tests to your PR but here is my contribution.

In the README.md file, I'd add the following section under Usage:

### Library

You can run dbmate inside your go program by using it as a library.

The library mode also supports the [`go:embed` directive](https://pkg.go.dev/embed),
which can be used to embed the migration files inside your go binary.

It can be also be used to run your migrations in your unit tests.

Here is an example:

package main

// The following example shows how to load migrations with go:embed,
// which enables you to embed the migrations files inside your binary.
// This can also be used to run the migrations in your tests.

// In order for this example to work, you should have a directory named migrations,
// which contains your migration files.

// The program will apply all migrations.

import (
	"embed"
	"fmt"
	"log"
	"net/url"
	"os"

	"github.com/amacneil/dbmate/pkg/dbmate"
	// load dbmate postgres driver.
	_ "github.com/amacneil/dbmate/pkg/driver/postgres"
	_ "github.com/joho/godotenv/autoload"
)

//go:embed migrations
var migrationFS embed.FS

func migrate() error {
	dbURL, err := url.Parse(os.Getenv("DATABASE_URL"))
	if err != nil {
		return fmt.Errorf("parse DATABASE_URL: %w", err)
	}

	mate := dbmate.New(dbURL)
	mate.FS = migrationFS
	mate.MigrationsDir = "migrations"
	mate.AutoDumpSchema = false

	err = mate.CreateAndMigrate()
	if err != nil {
		return fmt.Errorf("mate.CreateAndMigrate: %w", err)
	}
	return nil
}

func main() {
	err := migrate()
	if err != nil {
		log.Fatal(err.Error())
	}
}

Feel free to add it to your branch, so the PR is ready.

Thanks!

@gnuletik
Copy link
Contributor

@Enrico204 what do you think about the doc / example?
If you prefer, I can create another PR from yours.

@amacneil
Copy link
Owner

I had some concerns with the approach in this PR and started a different attempt, but have not had time to clean up / push it yet.

@gnuletik
Copy link
Contributor

Thanks for your feedback @amacneil! Can you expand on your concerns ? I may help for the implementation.

@Enrico204
Copy link
Contributor Author

@Enrico204 what do you think about the doc / example? If you prefer, I can create another PR from yours.

Sorry, I was quite busy in these weeks - it looks good to me :-)

@amacneil I am interested in your comments :-) I didn't like this approach too, but I had no clue on how to implement this at the time.

Maybe I can create a dbmate.NewEmbed(embed.FS) or something like that, what do you think? It may be more idiomatic.
@gnuletik ?

@gnuletik
Copy link
Contributor

@Enrico204 I like the dbmate.NewEmbed approach!

However, we would also need to add a basePath param (directory where the migration files are located).

E.g.

dbmate.NewEmbed(embedFS, basePath)

This would help making AutoDumpSchema disabled here (because embed.FS is read-only).

@Enrico204
Copy link
Contributor Author

@Enrico204 I like the dbmate.NewEmbed approach!

However, we would also need to add a basePath param (directory where the migration files are located).

E.g.

dbmate.NewEmbed(embedFS, basePath)

This would help making AutoDumpSchema disabled here (because embed.FS is read-only).

Uhm, I think that the basePath is not needed for embedding (nor is MigrationsDir anymore): one can use fs.Sub() instead, before passing the fs.FS to dbmate.NewEmbed. Then, dbmate can look for all migrations in the root of the virtual filesystem.

Something like:

//go:embed migrations
var migrationFS embed.FS

func main() {
	// ...
	subFS, _ := fs.Sub(migrationFS, "my_migrations_dir")

	mate := dbmate.New(dbURL, subFS)

	err = mate.CreateAndMigrate()
	// ...
}

@gnuletik
Copy link
Contributor

@amacneil Can you let us know how can we get this PR merged please?

@Enrico204
Copy link
Contributor Author

@amacneil Can you let us know how can we get this PR merged please?

I think that it's wise to discuss and agree on a design that is ok before implementing this change, as it might be difficult to change later. In the meantime, you can use your own fork (like I'm doing).

@amacneil amacneil force-pushed the support-migrations-embedding branch 6 times, most recently from b116e86 to 8c2260f Compare February 21, 2023 00:52
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

Successfully merging this pull request may close these issues.

3 participants