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

Add http.FileSystem migration source driver support #293

Merged
merged 26 commits into from
Dec 24, 2019

Conversation

fln
Copy link

@fln fln commented Oct 21, 2019

This PR adds a new migration source driver capable of reading files from
any source that implements http.FileSystem interface.

Notable http.FileSystem interface implementations:

  • http.Dir() - wrapper over local file-system
  • github.com/shurcooL/vfsgen

Because user of this package is responsible for getting an
implementation of http.FileSystem, this driver does not support creating
instances from driver URLs.

@coveralls
Copy link

coveralls commented Oct 21, 2019

Pull Request Test Coverage Report for Build 596

  • 86 of 95 (90.53%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 51.944%

Changes Missing Coverage Covered Lines Changed/Added Lines %
source/errors.go 0 3 0.0%
source/httpfs/partialdriver.go 77 83 92.77%
Totals Coverage Status
Change from base Build 565: 0.8%
Covered Lines: 4836
Relevant Lines: 9310

💛 - Coveralls

@wayneashleyberry
Copy link

wayneashleyberry commented Nov 6, 2019

Looks like markbates/pkger also implements http.FileSystem, I'd definitely consider that a notable implementation as it is essentially a replacement for gobuffalo/packr.

https://blog.gobuffalo.io/introducing-pkger-static-file-embedding-in-go-1ce76dc79c65

This PR adds a new migration source driver capable of reading files from
any source that implements http.FileSystem interface.

Notable http.FileSystem interface implementations:

* http.Dir() - wrapper over local file-system
* github.com/shurcooL/vfsgen

Because user of this package is responsible for getting an
implementation of http.FileSystem, this driver does not support creating
instances from driver URLs.
This PR adds a new httpfs source driver constructor New(). It is
identical to the WithInstance() constructor but will postpone any errors
until the driver is actually being used.

This allows clients of this package to have less error checks without
loosing correctness.

In addition this PR adds more tests, improves test code coverage, and
makes golinter happy by removing deferred Close call.
Copy link
Member

@dhui dhui left a 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!
This is a good start, but the interface for httpfs.driver should change. Namely it shouldn't implement the source.Driver interface but instead be embedded by other structs which fully implement the source.Driver interface.

source/httpfs/coverage.out Outdated Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/driver_test.go Outdated Show resolved Hide resolved
source/httpfs/driver_test.go Outdated Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/faileddriver.go Outdated Show resolved Hide resolved
@dhui
Copy link
Member

dhui commented Dec 11, 2019

@fln Don't force push to your branch since it makes PRs harder to review
I'll re-run any failing tests that aren't due to your changes

@fln
Copy link
Author

fln commented Dec 12, 2019

Thanks @dhui for reviewing and helping to push this PR forward. We are heavy users of golang-migrate/migrate package and happy to be able to contribute.

source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/driver.go Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/README.md Outdated Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
Copy link
Member

@dhui dhui left a 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! Just a few more issues.
We should split up httpfs.HTTPFS and httpfs.HTTPFSPassThrough structs.
Examples/docs should be updated accordingly and showcase embedding.

source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/driver_test.go Outdated Show resolved Hide resolved
@fln
Copy link
Author

fln commented Dec 13, 2019

Updated the PR to expose httpfs.Migrator for embedding into new source drivers and New() for http.FileSystem conversion to source.Driver. If you approve current package API I could tidy it up, squash commits and create new clean PR.

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the last round of feedback!

If you approve current package API I could tidy it up, squash commits and create new clean PR.

I don't think there's a need to squash commits or open a new PR since I'll do a squash an merge via github. If there's any other tidying you can think of, please feel free to do so

source/httpfs/README.md Outdated Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
source/httpfs/driver_test.go Outdated Show resolved Hide resolved
source/httpfs/driver_test.go Outdated Show resolved Hide resolved
source/httpfs/driver_test.go Outdated Show resolved Hide resolved
source/httpfs/migrator.go Outdated Show resolved Hide resolved
source/httpfs/driver.go Outdated Show resolved Hide resolved
@fln fln requested a review from dhui December 19, 2019 13:58
@fln
Copy link
Author

fln commented Dec 19, 2019

All review issues are resolved for the time being 🕺.

// ErrDuplicateMigration is an error type for reporting duplicate migration
// files.
type ErrDuplicateMigration struct {
Filename string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a source.Migration and an os.File instead of a filename string. I'll fix this myself to avoid an extra review roundtrip

@dhui dhui merged commit 09f532b into golang-migrate:master Dec 24, 2019
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.

None yet

4 participants