Skip to content

Conversation

@klassicd
Copy link
Contributor

Fixes #168

Replaces async.each with async.eachSeries in automigrate() and autoudate() so migrations are run in a series instead of in parallel. This enforces the model order so foreign key constructs with Postgres are possible.

Previously it was impossible to enforce order in Loopback 4 using the models option to migrateSchema({models: ['m1', 'm2']}) as documented here https://loopback.io/doc/en/lb4/todo-list-tutorial-sqldb.html

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Commit messages are following our guidelines

Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

@klassicd, thanks for your PR. Your changes LGTM. I'd like to get at least one more review before landing. Thanks.

@dhmlau
Copy link
Member

dhmlau commented Feb 24, 2020

@slnode test please

@dhmlau dhmlau added the community-contribution Patches contributed by community label Feb 24, 2020
Copy link

@montera82 montera82 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@klassicd Thank you for the contribution, looks reasonable to me. I left a minor comment for the test.


it('automigrate reports errors for models not attached', function(done) {
ds.automigrate(['m1', 'm2'], function(err) {
ds.automigrate(['m1', 'm3'], function(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason change m2 to m3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously there was only a single table m1 attached and m2 was un-attached. The new test (line #55) expects two tables created in a series. So I refactored two tables m1 and m2 to be attached by default, and here to look for an un-attached table m3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the explanation, fair enough.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

Thank you @klassicd approved. I can help you rebase the PR and will double check the downstream failures.

@klassicd
Copy link
Contributor Author

Yes @jannyHou that would help, thank you.

@jannyHou
Copy link
Contributor

@slnode test please

@dhmlau dhmlau merged commit 49e4c37 into loopbackio:master Apr 1, 2020
@dhmlau
Copy link
Member

dhmlau commented Apr 1, 2020

@klassicd, thanks for your contribution. Your PR has landed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Patches contributed by community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Automigrate & Autoupdate should use async.eachSeries

4 participants