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

V2 #3

Merged
merged 17 commits into from
Jan 23, 2018
Merged

V2 #3

merged 17 commits into from
Jan 23, 2018

Conversation

josephbuchma
Copy link
Member

@josephbuchma josephbuchma commented Dec 26, 2017

Quite massive backward-incompatible refactoring :)
All drivers and journey cli are also updated.
This PR addresses all concerns discussed in #2 (in one way or another)

Changes

  • Removed all "async" stuff.
  • Added type for encapsulating migrations functionality for greater flexibility and performance.
  • Changed driver registration and initialization approach, fixed Driver must be stateless or not a singleton #5
    • Removed Initialize method from Driver interface
    • Removed FlenameExtension from Driver interface
  • Added driver.Locker interface, which can be optionally implemented to enable locking during migraitons.
    • Implemented for mysql driver
  • Migrate now receives context.Context, and therefore can be cancelled.
  • Added option to attach pre/post hooks for migrations.
  • Added methods for applying / rolling back specific version.

@gravis
Copy link
Member

gravis commented Dec 26, 2017

A lot of things to review :)
Please give me a few days

@josephbuchma
Copy link
Member Author

josephbuchma commented Dec 27, 2017

@gravis tests on master branch are failing

❯❯❯ make test                                                                   ⏎ master ✱
docker-compose run --rm go-test
Starting migrate_crate_1
=== RUN   TestCreate
--- PASS: TestCreate (0.24s)
        migrate_test.go:31: Test driver: postgres://[email protected]:5432/template1?sslmode=disable
        migrate_test.go:31: Test driver: mysql://root@tcp(172.17.0.9:3306)/migratetest
        migrate_test.go:31: Test driver: sqlite3:///tmp/migrate.db
=== RUN   TestReset
--- PASS: TestReset (0.73s)
        migrate_test.go:72: Test driver: postgres://[email protected]:5432/template1?sslmode=disable
        migrate_test.go:72: Test driver: mysql://root@tcp(172.17.0.9:3306)/migratetest
        migrate_test.go:72: Test driver: sqlite3:///tmp/migrate.db
=== RUN   TestDown
--- FAIL: TestDown (0.17s)
        migrate_test.go:110: Test driver: postgres://[email protected]:5432/template1?sslmode=disable
        migrate_test.go:140: Expected version 0, got 20171227092036
=== RUN   TestUp
--- FAIL: TestUp (0.05s)
        migrate_test.go:147: Test driver: postgres://[email protected]:5432/template1?sslmode=disable
        migrate_test.go:165: Expected version 0, got 20171227092036
=== RUN   TestRedo
--- PASS: TestRedo (1.03s)
        migrate_test.go:189: Test driver: postgres://[email protected]:5432/template1?sslmode=disable
        migrate_test.go:189: Test driver: mysql://root@tcp(172.17.0.9:3306)/migratetest
        migrate_test.go:189: Test driver: sqlite3:///tmp/migrate.db
=== RUN   TestMigrate
--- FAIL: TestMigrate (0.18s)
        migrate_test.go:229: Test driver: postgres://[email protected]:5432/template1?sslmode=disable
        migrate_test.go:267: Versions in db: [20171227092036 20171227092035 20171227092034 20171227092033 20171227092032 20171227092031 20171227092030 20171227092029 20171227092028 20171227092027 20171226235039 20171226235038 20060102150405]
        migrate_test.go:268: Expected version 0, got 20171227092036
FAIL
exit status 1
FAIL    github.com/db-journey/migrate   2.399s
?       github.com/db-journey/migrate/direction [no test files]
?       github.com/db-journey/migrate/driver    [no test files]
=== RUN   TestParseFilenameSchema
--- PASS: TestParseFilenameSchema (0.00s)
=== RUN   TestFiles
--- PASS: TestFiles (0.00s)
=== RUN   TestDuplicateFiles
--- PASS: TestDuplicateFiles (0.00s)
PASS
ok      github.com/db-journey/migrate/file      0.003s
?       github.com/db-journey/migrate/pipe      [no test files]
Makefile:7: recipe for target 'test' failed
make: *** [test] Error 1

* Added driver.Lockable interface, which can be optionally implmented
  in order to make it safe to use concurrently.
@josephbuchma
Copy link
Member Author

josephbuchma commented Dec 27, 2017

@gravis I added optional pre/post hooks for migraitons.
Useful for automating more complex migrations, and it also can be used for progress logging on client side (CLI).

@josephbuchma
Copy link
Member Author

josephbuchma commented Dec 28, 2017

@gravis just want to note that I think I addressed all concerns discussed in #2 (in one way or another)

@josephbuchma
Copy link
Member Author

Happy New Year !

Waiting for your feedback @gravis :)

@josephbuchma
Copy link
Member Author

ping

@gravis
Copy link
Member

gravis commented Jan 8, 2018

Hi there. I'm back to work today, sorry for the delay.
I'll review your PR asap.
Thanks

Copy link
Member

@gravis gravis left a comment

Choose a reason for hiding this comment

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

LGTM.
Handle name is certainly not blocking.
Let's continue with changes in drivers, and we'll be good to merge

}

// Handle encapsulates migrations functionality
type Handle struct {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we name it Handler, instead of Handle?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like Handler b/c it's common name/pattern which usually refers to interface or function.
We can go conservative way and name it Migrate . I was simply looking for something that plays well with migrate. prefix and isn't counter-intuitive (at least), and Handle is the best what I came up with.

@josephbuchma
Copy link
Member Author

josephbuchma commented Jan 15, 2018

round 2 @gravis 😄
This time it's a bit more radical. I decided to fix #5 and it went a bit further.
All drivers are up to date in respective PRs

@josephbuchma
Copy link
Member Author

@gravis now I think I'm happy with results. From my perspective it's ready for merge, and drivers can be polished with follow-up PRs.

@gravis gravis merged commit c7271af into db-journey:master Jan 23, 2018
@gravis
Copy link
Member

gravis commented Jan 23, 2018

Thanks for your tremendous work!

@josephbuchma josephbuchma mentioned this pull request Mar 19, 2018
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.

Driver must be stateless or not a singleton
2 participants