v2 #9
v2 #9
Conversation
|
LGTM. |
|
hmm, the api doesn't change here, no need for v3, ignore my last comment. |
|
It still needs small updates, like bringing back migrations progress logging, or "press Ctrl-c to interrupt immediately" etc. |
|
Thanks for all your changes. |
Also moved to using instance methods rather than migrate package funcs.
|
Now all original behavior is restored (including progress logging, ctrl-c to stop & repeat to force stop) New featuresapply specific version (run up migration) |
|
You're changing too many things at once, it's becoming harder and harder to merge... :) |
| func newOsInterruptCtx(done <-chan struct{}) (context.Context, func()) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| c := make(chan os.Signal, 1) | ||
| signal.Notify(c, os.Interrupt) |
There was a problem hiding this comment.
A lib should never presume of context behaviour.
You must pass a context to migrate, and handle this stuff in https://github.com/db-journey/journey/ instead.
That way, you let the users chose where context.Background() is created.
There was a problem hiding this comment.
I thought that commands package is not supposed to be used apart from journey CLI. And it doesn't presume context behavior. The only thing it "presumes" is that context might be canceled, which is kind of normal thing to presume when you work with context.
So passing context here from journey or whatever else won't change/break anything.
There was a problem hiding this comment.
If you talking about os signals processing - yes, it does makes sense to move it to journey's main package if commands is used elsewhere beyond journey.
There was a problem hiding this comment.
my bad, I mixed up with the migrate project.
You're right!
| return nil | ||
| }, | ||
| } | ||
|
|
||
| func logErr(err error) *log.Entry { | ||
| return log.WithError(err) | ||
| func newMigrateWithCtx(url, migrationsPath string) (*migrate.Handle, context.Context, func()) { |
There was a problem hiding this comment.
So, why the context.Context is not a field of *migrate.Handle?
I smell a wrong utilization of context here.
There was a problem hiding this comment.
It shouldn't be a field of *migrate.Handle. If you add context to Handle at create time, it will make this Handle useless once you cancel this context.
Compiles with db-journey/migrate#3
UPDATE
Now all original behavior is restored (including progress logging, ctrl-c to stop & repeat to force stop)
Also got rid of migrate "shortcut" functions.
New features