Skip to content

Conversation

@greg-rychlewski
Copy link
Contributor

This can be useful for example when the user wants to have advisory locks for migrations but doesn't need that level of locking for checking pending migrations.

@josevalim
Copy link
Member

Woot! Nice seeing you here too @greg-rychlewski!

I think we need to bump the Ecto dependency to ~> 3.5 in the mix.exs file. Can you please do that?

@greg-rychlewski
Copy link
Contributor Author

Oh yes sure no problem :). All done.

Is it ok that the mix.lock version is 3.9 or should that be 3.5?

@josevalim
Copy link
Member

It is fine to be 3.9 I think!

@josevalim
Copy link
Member

It seems we will also have to bump CI and mix.exs to use at least Elixir v1.10. :)

@greg-rychlewski
Copy link
Contributor Author

Updated the CI to Elixir 1.10/OTP 21 and Elixir 1.14/OTP 25

@josevalim josevalim merged commit 4aa366c into phoenixframework:master Dec 2, 2022
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@dbernheisel
Copy link

you beat me to it :)

@dkarter
Copy link
Contributor

dkarter commented Sep 11, 2025

Hey sorry for resurface this, but we just spent a whole day debugging this issue 😅. We were pretty happy that we found this solution, but were wondering - is there a reason this is not the default?

I'm happy to submit a PR to make it the default if that makes sense.

@SteffenDE
Copy link
Contributor

@dkarter what exactly do you mean when you say "spent a whole day debugging this issue"? So what behavior did you see that was unexpected?

@dbernheisel
Copy link

This issue describes the issue pretty well:
fly-apps/safe-ecto-migrations#6

@SteffenDE
Copy link
Contributor

I see. Sadly, I don't know why CheckRepoStatus uses locking, but I assume there's a good reason. So rather than disabling it, maybe we should detect this and emit a warning? Or rather default to :table_lock? cc @josevalim

@josevalim
Copy link
Member

Since we are just checking migrations, we can probably have migration_lock: false by default for checking?

@dkarter
Copy link
Contributor

dkarter commented Sep 12, 2025

I have a PR up here: #196

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.

5 participants