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

use mutable list to register migrations #568

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timohuber
Copy link

No description provided.

@timohuber
Copy link
Author

@aronerben Updated PR using a mutable list to store the registered migration.

For some reason, the migrations are registered in the reversed order, we register the services.

For example:

let services =
  [ Service.Migration.register (Database.Migration.migrations ())
  ; Service.Token.register ()
  ; Service.User.register ()
  ; Service.Email.register ()
  ]
;;

Will create migrations in the following order:

email
user
token
....

Do you know why this behaves like this?

@aronerben
Copy link
Contributor

aronerben commented May 29, 2024

@timohuber Hmm, the implementation seems correct. Could you figure out where this reversal happens by logging? For example, is it reversed in let execute ?ctx migrations = ... already?

@aronerben aronerben marked this pull request as draft May 29, 2024 15:10
@aronerben aronerben self-requested a review May 29, 2024 15:10
@timohuber
Copy link
Author

@aronerben Sorry, I was unclear. This behavior is not directly related to this PR.

The services are started in a different order than passed to the with_services function and, therefore, the migrations. They are sorted in top_sort_lifecycles. Do you know what the reason is to sort the lifecycles?

@aronerben
Copy link
Contributor

@timohuber
Ah, I see. top_sort_lifecycles is necessary to sort the services based on their dependencies. So user depends on email (as each user should have an email address), which implies that email should be loaded before user. I guess the migrations are also run before.

@timohuber
Copy link
Author

@aronerben Yes, you are right. migrations are run first. Do you have an idea how we can ensure that all migrations from sihl are executed before the one registered by the app? Store them into two different variables, as the first draft did?

The current workaround is to prefix them to make sure they are executed last in alphabetical order.

@aronerben
Copy link
Contributor

aronerben commented Jun 4, 2024

@timohuber Technically, if you register your own services and make them dependent on the corresponding Sihl service it depends on, the migration should be run after the Sihl migration...

Would that work? Otherwise we just separate Sihl migrations and custom migrations via the way you suggested.

(Of course, the cleanest solution would be to rework the migration system, as was planned in Sihl 4.0...)

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.

2 participants