-
Notifications
You must be signed in to change notification settings - Fork 1
add cigogne support #2
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
base: main
Are you sure you want to change the base?
Conversation
|
Hey, thanks for this! Have you tested the migration works in both directions? From a cursory glance it looks like you might need a Also, this will break the current migration CLI, I believe. I don't intend to force folks to use Cigogne if they're already using another tool for migrations. |
|
Going I'll admit I tunnel-visioned a bit and missed the existing migration CLI; I'm not entirely sure how to handle that, the first thing that comes to mind is to split on |
|
It's possible that Cigogne has something we can use to just get the text from the up migration. I haven't looked at the API for this for a while. |
|
They do (https://hexdocs.pm/cigogne/cigogne.html#get_all_migrations), but it comes in a List(String) of queries so the CLI would need a bit of a rework. |
|
That looks like a sensible approach |
|
Done, if I haven't missed something. |
|
Hey, apologies that I haven't gotten to this yet. I haven't forgotten, just been super busy. I'll try to get to it in the next couple of weeks. |
|
No worries! |
isaacharrisholt
left a comment
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.
Ideally want to avoid breaking changes here.
| use config <- result.try( | ||
| config.get("m25") | ||
| |> result.map_error(fn(err) { | ||
| err | ||
| |> string.inspect | ||
| |> add_error_context("Failed to get migrations configuration") | ||
| }), | ||
| ) |
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.
When I run this:
gleam run -m m25 migrate --addr=postgres://postgres:postgres@localhost:5432/postgresI get the following:
Getting migrations for m25 tables... Command failed with an error: Failed to get migrations to apply for m25 tables: Failed to create migrations engine: DatabaseError(EnvVarUnset("DATABASE_URL")
Requiring DATABASE_URL is a breaking change I'd prefer to avoid. I don't mind supporting it, but it has to be alongside the --addr flag, with the flag taking precedence. I think you could do this in the connection_string_flag function where the flag's default is:
result.unwrap(
envoy.get("DATABSE_URL"),
"postgresql://postgres:postgres@localhost:5432/postgres",
)You'd need to always pass the configured DB URL into Cignogne then.
No description provided.