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

Setting context on migration transactions #195

Closed
wants to merge 1 commit into from

Conversation

diogogmt
Copy link

This PR allows a context to be set in the database transactions used to run .go and .sql migrations.

db.BeginTx is called Instead of db.Begin which allows the caller the following options:

  • create a context with a deadline/timeout
  • cancel transactions with a bad state/lock
  • clean up existing transactions if the goose process is terminated before the migrations are completed

@mfridman
Copy link
Collaborator

I wonder if folks in the community are using the goose helper functions directly, e.g., Down, DownTo etc.

It may be safer to either cut a new v3.0.0 or introduce new functions to keep backwards compat.

@VojtechVitek let's chat when you have some time.

@ilazakis
Copy link

We’re using all of these methods (Up, Down etc) to run migrations from Go apps.

Introducing new methods is probably the best way to go. A v3 may make it clear that there’s breaking changes but won’t remove the element of surprise and extra work for upgrades. It will also require changes for anyone wishing to get access to features and fixes post v3.

@c9s
Copy link

c9s commented Jan 17, 2020

fyi: merged in c9s/goose@472052d

Copy link
Collaborator

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

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

I like this contribution. However, it's breaking the API.

We'll need to add new context-aware functions, ie. DownCtx()

Copy link
Collaborator

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

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

@mfridman it's too bad we never got to merging this.

I think adding the extra *Ctx() function is fine and doesn't break the existing API. I like this PR.

@mfridman
Copy link
Collaborator

Agreed. Not sure if @diogogmt wants to see this one through (3 years later 😅) .. but I can pick this up and commandeer it to completion.

I think there is value in having this be an option in the current branch without breaking the API.

@diogogmt
Copy link
Author

diogogmt commented Jul 6, 2022

@mfridman it has been a while!

I can update the PR. We have been running a forked version propagating the context cancelation signal to the SQL statements here at Blockthrough. It would be great to have it merged upstream.

@michaljemala
Copy link

@VojtechVitek Is there anything blocking this change? Can I help somehow?

@mfridman mfridman mentioned this pull request May 8, 2023
7 tasks
@ori-shalom
Copy link
Contributor

I wasn't aware of this PR and opened #517 with the same intention.
Since this PR is already quite old and #517 is based on the current master (v3), maybe it is worth closing this one and continuing on my PR instead?
Both are essentially accomplishing the same thing which is adding new methods for passing Go context to migrations while keeping the old methods for backward compatibility.

@mfridman
Copy link
Collaborator

Superseded by #517

@mfridman mfridman closed this Jun 16, 2023
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

7 participants