-
Notifications
You must be signed in to change notification settings - Fork 13
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(migrators): add ternmigrator for jackc/tern #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting on WIP. I asked a few questions, but besides the CI failure, this looks fantastic and I'd be OK with merging it. Thanks for the quick turnaround.
migrators/goosemigrator/README.md
Outdated
@@ -13,8 +13,8 @@ filesystem. | |||
[Golang-defined migrations](https://github.com/pressly/goose#go-migrations) are | |||
not supported by default. | |||
|
|||
You can configure the migrations directory, the table name, and the filesystem | |||
being used. Here's an example: | |||
[You can configure the migrations directory, the table name, and the filesystem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidental change?
migrators/ternmigrator/README.md
Outdated
for _, test := range []struct { | ||
name string | ||
fs fs.FS | ||
}{ | ||
{name: "FromDisk"}, | ||
{ | ||
name: "FromFS", | ||
fs: exampleFS, | ||
}, | ||
} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind updating this to follow the pattern of the existing examples, instead of this two-item for loop? Entirely subjective, not a blocker, I would just prefer it that way.
migrators/ternmigrator/tern.go
Outdated
package ternmigrator | ||
|
||
import ( | ||
"cmp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen this error before, but can you check the go.mod
/go.sum
files by running go mod tidy
and confirming that the tests pass locally? This may be a CI-only issue, in which case I'll dig in further to figure out how to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmp was introduced in go1.22. I wasn't thinking about backward compatibility on this part.
migrators/ternmigrator/tern.go
Outdated
|
||
var _ pgtestdb.Migrator = (*TernMigrator)(nil) | ||
|
||
const defaultTableName = "public.schema_version" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not export this (DefaultTableName
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because I didn't think about it and I'm generally a minimalist on exports. I exported it in my latest commit.
migrators/ternmigrator/tern_test.go
Outdated
name: "FromFS", | ||
fs: exampleFS, | ||
}, | ||
} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same preference for two separate testcases rather than a two-item for loop. Similarly not a blocker, just a request.
@peterldowns can you have another look? |
@WillAbides Thank you for your patience — this is great! I only made a few small tweaks to the documentation and git-tagging scripts I use to maintain this repo. Thank you for your contribution! |
This PR adds a
ternmigrator
package for use with jackc/tern. It is based on the patterns established bygoosemigrator
andbunmigrator
, and supports:This PR closes #10