-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 io/fs#FS Driver #471 #472
Conversation
Pull Request Test Coverage Report for Build 929
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
You'll need to add an internal/cli/build_iofs.go
file and update Makefile
with the build constraint for the official CLI to support iofs
.
source/iofs/README.md
Outdated
|
||
Driver with file system interface (`io/fs#FS`) supported from Go 1.16. | ||
|
||
This Driver cannot be used with versions below Go 1.15. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update to: This Driver cannot be used with Go versions 1.15 and below
source/iofs/iofs.go
Outdated
|
||
// Iofs is a source driver for io/fs#FS. | ||
type Iofs struct { | ||
httpfs.PartialDriver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source/iofs/iofs.go
Outdated
// WithInstance wraps io/fs#FS as source.Driver. | ||
func WithInstance(fsys fs.FS, path string) (source.Driver, error) { | ||
var i Iofs | ||
if err := i.Init(http.FS(fsys), path); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting from fs.FS
to http.FileSystem
may fail later if the underlying struct doesn't implement io.Seeker
. Seek()
isn't used in the httpfs
source driver right now, but if that changes, this would break.
See: https://tip.golang.org/src/net/http/fs.go
func (f ioFile) Seek(offset int64, whence int) (int64, error) {
s, ok := f.file.(io.Seeker)
if !ok {
return 0, errMissingSeek
}
return s.Seek(offset, whence)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating this to not depend on httpfs!
source/iofs/iofs.go
Outdated
} | ||
|
||
// Iofs is a source driver for io/fs#FS. | ||
type Iofs struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to IoFS
since abbreviations should be in all caps.
source/iofs/iofs.go
Outdated
|
||
// Init prepares not initialized Iofs instance to read migrations from a | ||
// fs.ReadDirFS instance and a relative path. | ||
func (p *Iofs) Init(fsys fs.ReadDirFS, path string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use fs.ReadDirFS
instead of fs.FS
? Does using ReadDir
directly through the FS save iops? e.g. don't need to open a file, stat it, and finally readdir?
Does this optimization reduce compatibility with different filesystems?
We're probably better off using fs.ReadDir
source/errors.go
Outdated
// ErrDuplicateMigration is an error type for reporting duplicate migration | ||
// files. | ||
type ErrDuplicateMigration struct { | ||
Migration | ||
os.FileInfo | ||
FileInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use os.FileInfo
or fs.FileInfo?
You can use fs.DirEntry.Info() to get the fs.FileInfo
source/iofs/iofs.go
Outdated
} | ||
file, err := e.Info() | ||
if err != nil { | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is continue OK like any other error case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://tip.golang.org/pkg/io/fs/#DirEntry an error in this case may mean:
If the file has been removed or renamed since the directory read, Info may return an error satisfying errors.Is(err, ErrNotExist).
But I think it can also just represent any other IO error, in e.g. the case the FS interface is backed by a real FS and not embed
. I think ignoring the error and continue
ing is wrong here, instead any IO error should be reported back to the caller of the function I think (also in case of DefaultParse
error above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that errors should be returned and not ignored. Ignored errors will make bugs harder to debug.
Thanks for the great review. |
… the changes in ErrDuplicateMigration
cf97126
to
e18e378
Compare
source/iofs/README.md
Outdated
|
||
Driver with file system interface (`io/fs#FS`) supported from Go 1.16. | ||
|
||
This Driver cannot be used with Go versions 1.15 and below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Reword this to something like just:
Package iofs provides a Go 1.16+
io/fs
driver which is compatible with Go 1.16+ file//embed
directives.
Side note: I think all the documentation in this file would be better placed into a package-level godoc documentation comment, so that on https://pkg.go.dev this package shows up with package docs and usage examples similar to this and this
Feel free to ignore if this is not helpful :)
source/iofs/README.md
Outdated
"github.com/golang-migrate/migrate/v4/source/iofs" | ||
) | ||
|
||
//go:embed migrations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//go:embed migrations | |
//go:embed migrations/*.sql |
May be useful to describe this to avoid people from accidentally embedding their own README.md
, .gitignore
, .DS_Store
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my feedback!
source/iofs/iofs.go
Outdated
|
||
// Init prepares not initialized IoFS instance to read migrations from a | ||
// fs.FS instance and a relative path. | ||
func (p *IoFS) Init(fsys fs.FS, path string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts about moving most of this implementation to a PartialDriver
, similar to httpfs
? We'd also need to rename WithInstance()
to New()
and changeIoFS
to driver
. That way other drivers can embed the partial driver and implement their own Open()
receiver function.
.golangci.yml
Outdated
@@ -1,6 +1,6 @@ | |||
run: | |||
# timeout for analysis, e.g. 30s, 5m, default is 1m | |||
timeout: 2m | |||
timeout: 5m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave this at 2m
I'll re-run any CI jobs/tests that fail due to timeout issues.
//go:embed testdata/migrations/*.sql | ||
var fs embed.FS | ||
|
||
func Example() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for using executable examples!
source/file/file.go
Outdated
url string | ||
path string | ||
} | ||
type File = file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a type alias necessary? I think you can define type struct File
in both the go 1.16 and non-1.16 files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of struct do you envision?
Sorry, I can't understand well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can avoid the type alias by doing:
// In file_go115.go
type File struct {
httpfs.PartialDriver
url string
path string
}
// In file_go116.go
type File struct {
iofs.PartialDriver
url string
path string
}
source/file/file_go116.go
Outdated
) | ||
|
||
type file struct { | ||
iofs.Driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
source/iofs/iofs.go
Outdated
) | ||
|
||
// Driver is a source driver that wraps io/fs#FS. | ||
type Driver struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't export this. e.g. rename to driver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If did not export it, you will not be able to embed it in the file driver.
httpfs.PartialDriver is public now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand it, I will separate into PartialDriver
and driver
.
source/iofs/iofs.go
Outdated
} | ||
|
||
// NewDriver returns a new Driver from io/fs#FS and a relative path. | ||
func NewDriver(fsys fs.FS, path string) (source.Driver, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to New
to follow the same pattern as httpfs.New()
source/iofs/iofs.go
Outdated
|
||
// Init prepares not initialized IoFS instance to read migrations from a | ||
// io/fs#FS instance and a relative path. | ||
func (d *Driver) Init(fsys fs.FS, path string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move all of the driver receiver methods (except for Open()
) to PartialDriver
to follow the same patter than httpfs uses.
This reverts commit 2cd12a6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the PartialDriver
change! One last thing and we're good to go!
source/iofs/iofs.go
Outdated
func (d *Driver) Open(url string) (source.Driver, error) { | ||
panic("iofs: Driver does not support open with url") | ||
func (d *driver) Open(url string) (source.Driver, error) { | ||
panic("iofs: driver does not support open with url") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't panic and instead return an error: return nil, errors.New("Open() cannot be called on the iofs passthrough driver")
This reverts commit fedbe42.
|
||
func Test(t *testing.T) { | ||
//go:embed testdata/migrations/*.sql | ||
var fs embed.FS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curious--how did this even work?
Per the golang docs:
The //go:embed directive can be used with both exported and unexported variables, depending on whether the package wants to make the data available to other packages. It can only be used with global variables at package scope, not with local variables.
To me this looks like a local variable, not a global variable at package scope... What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local variables were still supported when implementing this PR.
golang/go#43216
It is a remnant of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thank you!
close #471