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.

Sign up for GitHub

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

Jump to bottom

Update migration.js #228

Merged
merged 1 commit into from
May 12, 2020
Merged

Update migration.js #228

merged 1 commit into from
May 12, 2020

Conversation

ataft
Copy link
Contributor

@ataft ataft commented Mar 27, 2020

Execute alterTable statements in order

@slnode
Copy link

slnode commented Mar 27, 2020

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@ataft ataft mentioned this pull request Mar 27, 2020
Closed
@dhmlau
Copy link
Member

dhmlau commented Mar 28, 2020

@slnode test please

@dhmlau dhmlau added the community-contribution Patches contributed by community label Mar 28, 2020
@ataft ataft mentioned this pull request Apr 7, 2020
Closed
@ataft
Copy link
Contributor Author

ataft commented Apr 30, 2020

Any updates on this?

@agnes512
Copy link
Contributor

Could you add tests for the change? I think https://github.com/strongloop/loopback-connector-mssql/blob/master/test/autoupdate.test.js might be a good place 😄 thanks!

@ataft
Copy link
Contributor Author

ataft commented Apr 30, 2020

The current test already covers this PR because it does an automigrate (create the table) on line 135 and then an autoupdate on line 155, which will call alterTable. BTW, this test should be failing intermittently, but given that the order is random it'd be hard to catch.

@ataft ataft mentioned this pull request Apr 30, 2020
Closed
@agnes512
Copy link
Contributor

@slnode test please

@jannyHou
Copy link
Contributor

jannyHou commented May 6, 2020

@slnode test please

jannyHou
jannyHou approved these changes May 6, 2020
View reviewed changes
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.

👍 Executing migration queries in sequence makes sense to me.
Thank you @ataft for contributing the code!

@agnes512
Copy link
Contributor

agnes512 commented May 7, 2020

@slnode test please

@jannyHou
Copy link
Contributor

jannyHou commented May 7, 2020

dropping node 8 in #230, hope it fixes the error

@ataft @jannyHou
Update migration.js
eb143b0 
Execute alterTable statements in order
@jannyHou
Copy link
Contributor

@slnode test please

@jannyHou
Copy link
Contributor

rebased the PR, let me trigger a new build and merge

@jannyHou jannyHou merged commit 0145119 into loopbackio:master May 12, 2020
@jannyHou
Copy link
Contributor

merged 🎉 thank you @ataft for the contribution again!

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

@agnes512 agnes512 agnes512 approved these changes

@jannyHou jannyHou jannyHou approved these changes

@dhmlau dhmlau Awaiting requested review from dhmlau dhmlau is a code owner

@emonddr emonddr Awaiting requested review from emonddr

Assignees
No one assigned
Labels
community-contribution Patches contributed by community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ataft @slnode @dhmlau @agnes512 @jannyHou