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

feat: allow Go migrations outside of an SQL transaction #696

Merged
merged 4 commits into from
Jun 26, 2023
Merged

Conversation

alnr
Copy link
Contributor

@alnr alnr commented Jun 16, 2023

No description provided.

@alnr alnr requested review from hperl, zepatrik and jonas-jonas June 16, 2023 11:47
@alnr alnr self-assigned this Jun 16, 2023
popx/migrator.go Show resolved Hide resolved
@alnr alnr requested a review from aeneasr June 26, 2023 08:07
Copy link
Collaborator

@hperl hperl left a comment

Choose a reason for hiding this comment

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

LGTM, with one small idea on how to improve the test :)

if _, err := c.Store.Exec("INSERT INTO tests (i, j) VALUES (?, ?)", i1, j1); err != nil {
return errors.WithStack(err)
}
close(up1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This took me a while to understand. You are passing control back here to see if the changes take effect immediately, right? Could you not just execute a closure instead which connection is bound to the top-level c?

Define in the top level:

assertValues := func(i int) {
  var j int64
  require.NoError(t, c.Store.Get(&j, "SELECT j FROM tests WHERE i = ?", i))
  assert.Equal(t, j1, j)

And then call it as part of the migrations.

Copy link
Member

Choose a reason for hiding this comment

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

From my side it's ok to merge as is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the closure would work too. Would be a little bit neater perhaps.

This works, so 🤷

@aeneasr aeneasr removed the request for review from jonas-jonas June 26, 2023 09:12
@aeneasr aeneasr merged commit 1c62a8a into master Jun 26, 2023
@aeneasr aeneasr deleted the gomigrations branch June 26, 2023 09:37
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.

3 participants