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
Add go1.16 embed #189
Add go1.16 embed #189
Conversation
This just adds a new struct with embed.FS and slightly changes findMigrations() to also have a root from where to start searching.
Yes, great, we need this! A version flag will be needed for as long as we support older Go (which we should for a while). Shouldn't be a problem though. |
for backwards compat with previous versions of the language the new code is protected with a +build tag.
Yup, done. Moved to a separate file. I also took the liberty to add Go 1.15 and 1.16 to the |
I tested the code locally with 1.15 and .16 and it passed. The failure on travis seems to be related to one of the dependencies and the stricter checking of go.mod. |
Awesome, thanks! |
Yeah they're no longer doing that automatically. Nothing problematic.
…On Mon, Feb 22, 2021, 11:29 Henry ***@***.***> wrote:
I tested the code locally with 1.15 and .16 and it passed.
The failure on travis
<https://travis-ci.org/github/rubenv/sql-migrate/jobs/759974466#L302>
seems to be related to one of the dependencies and the stricter checking of
go.mod.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#189 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAKPGGA2JQUXTMHRQZSYPLTAIWZBANCNFSM4X2JGQKQ>
.
|
Ah right I just realized this tests all the deps. Oh well.. :) Feel free to squash and merge this, then! |
Could that be fixed by upgrading the dependency? |
I guess so. The previous mssqldb was a commit from 2019. I bumped it to v0.9.0 but I have zero capacity to test that anywhere beyond checking that it builds. |
Oh well.. still fails. Actually not sure what is up there. I see |
I opened denisenkom/go-mssqldb#641 just in case. |
Thanks!
We can't just ignore it, if it fails for us it'll fail for our users as
well.
…On Mon, Feb 22, 2021, 14:03 Henry ***@***.***> wrote:
I opened denisenkom/go-mssqldb#641
<denisenkom/go-mssqldb#641> just in case.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#189 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAKPGDNZVDHQ4XWTLEH7W3TAJI3RANCNFSM4X2JGQKQ>
.
|
This looks great. Is there any work around the module issue for people who want to use this(before it's merged)? |
@rugwirobaker I don't think you will run into this last issue unless you test the dependencies with |
Upgrade to Go 1.16 and use the new embed package to make migrations available to Go. This involved using a fork of github.com/rubenv/sql-migrate until rubenv/sql-migrate#189 is merged, which adds support for the new Embed package to our migration tooling.
this will fix #129 ya? |
Great work, would really love to see this merged as well! While this is not the case tho, this worked for me (I have the migrations in the //go:embed migrations/*.sql
var migrations embed.FS
func runMigrations(db *sql.DB, steps int) int {
.
.
.
m := &migrate.AssetMigrationSource{
Asset: migrations.ReadFile,
AssetDir: func() func(string) ([]string, error) {
return func(path string) ([]string, error) {
dirEntry, err := migrations.ReadDir(path)
if err != nil {
return nil, err
}
entries := make([]string, 0)
for _, e := range dirEntry {
entries = append(entries, e.Name())
}
return entries, nil
}
}(),
Dir: "migrations",
}
.
.
.
n, err := migrate.ExecMax(db, "postgres", m, direction, steps)
if err != nil {
log.Fatal(err)
}
return n
} |
Hi there. I'd like suggest some notes. May be we'll not change previous methods and sources? And provide new I'll provide example below. import (
"bytes"
"errors"
"fmt"
"io/fs"
"strings"
)
// EmbedFS interface for supporting embed.FS as injected filesystem and provide possibility to mock.
type EmbedFS interface {
fs.ReadFileFS
}
// EmbedFileMigrationSource implements MigrationSource and provide migrations from a native embed filesystem.
type EmbedFileMigrationSource struct {
Filesystem EmbedFS
}
var _ MigrationSource = (*EmbedFileMigrationSource)(nil)
var (
ErrEmbedWalkFailed = errors.New(`failed to walk recursive over embed source directory`)
ErrEmbedReadDirFailed = errors.New(`directory read failed`)
)
// FindMigrations is part of MigrationSource implementation
func (f *EmbedFileMigrationSource) FindMigrations() ([]*Migration, error) {
items := make([]*Migration, 0)
err := fs.WalkDir(f.Filesystem, `.`, func(path string, entry fs.DirEntry, err error) error {
if err != nil {
return fmt.Errorf(`%v: %v`, ErrEmbedReadDirFailed, err)
}
// from now we always return nil cause if we send err, WalkDir stop processing
if entry.IsDir() {
return nil
}
if !strings.HasSuffix(entry.Name(), `.sql`) {
return nil
}
content, err := f.Filesystem.ReadFile(path)
if err != nil {
return nil
}
migration, err := ParseMigration(entry.Name(), bytes.NewReader(content))
if err != nil {
return nil
}
items = append(items, migration)
return nil
})
if err != nil {
return items, fmt.Errorf(`%v: %v`, ErrEmbedWalkFailed, err)
}
return items, nil
} This implementation does not depends on other FS or other implementations and just use internal package Also, |
This PR doesn't do that. It simply reuses the iteration code. The only blocking issue seems to be some testing headache that I can't fix since I don't have time to investigate it origin and don't have knowledge about MSSQL, which I already said weeks ago. Additionally it doesn't seem like the people on of package care much either 🤷♂️. Again, to me it seems very tangential to the changes here. People are welcome to test this branch with a replace statement. We have been using it since I opened this PR without any issues. |
And i suggest just write new dedicated code. As you see in my example - implementation is "easier" cause of we need only pass
Just need to update |
I went ahead and merged this, without the |
Not intended for merge but more as a discussion starter.All done with backwards compat!Since all the sources are in that big
migrate.go
I wasn't sure how to go about this but the new source could be pulled into a separate file with a// +build
so that the rest of the code still works on previous versions.Adding a root directory to
findMigrations()
was necessary becausedir.Open("/")
just returns a NotFound error and thusmigrationFromFile()
also had to change a little.