Skip to content

Compatibility with Sails v1 #36

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
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Compatibility with Sails v1 #36

wants to merge 3 commits into from

Conversation

@ochicf
Copy link

@ochicf ochicf commented Jun 3, 2019

  • added compatibility for Sails v1
  • allow to specify either connection or datastore option in configuration (to match sails v1 namings)
  • improved error messages

@ochicf
Copy link
Author

ochicf commented Jun 3, 2019

Old PR is a year old, I've written the changes requested plus some comments/docs/error improvements.

booyokkk
booyokkk suggested changes Jun 4, 2019
View reviewed changes
lib/sailsDbMigrate.js Outdated
connection = sailsConfig.connections[connectionName]
} else if (sailsConfig.datastores) {
// v1.x.x
sailsConfig.datastores[connectionName]
Copy link

@booyokkk booyokkk Jun 4, 2019

Choose a reason for hiding this comment

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

connection = sailsConfig.datastores[connectionName]

Copy link
Author

@ochicf ochicf Jun 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch, missed that after transforming it from the nested ternary operator.

Fixed in fa2506a

@leedm777
Copy link
Contributor

leedm777 commented Jun 5, 2019

I'm not using Sails any more. @booyokkk (or any other users of this lib), can I get a 👍 on this PR before I merge it?

@vshkr99
Copy link

vshkr99 commented Feb 19, 2021

@leedm777 can this PR be merged to master? I had the same compatibility issue with Sails v1 while creating migrations. Though I found the solution but since it already has an open PR, it would be convenient if we can have this PR merged since it has not conflicts with master.

naveedcs added a commit to naveedcs/sails-db-migrate that referenced this pull request Mar 26, 2021
@naveedcs
Update sailsDbMigrate.js
34c4dfa 
this will fix issue building5#36
@naveedcs naveedcs mentioned this pull request Mar 26, 2021
Closed
@mike-usa
Copy link

mike-usa commented Feb 23, 2022 •
edited
Loading

@leedm777 it looks like @booyokkk approved this last year. Any chance you could merge this in? I was going to recommend someone use node-pg-migrate for their sails app, but you all did such great work with this library that I'd like to test it out.

@naveedcs
Copy link

naveedcs commented Feb 23, 2022

Please ignore this pull request.

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

Reviewers

1 more reviewer

@booyokkk booyokkk booyokkk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ochicf @leedm777 @vshkr99 @mike-usa @naveedcs @booyokkk