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

add db:migrate:status #385

Merged
merged 1 commit into from
Nov 21, 2016
Merged

Conversation

gotrevor
Copy link

@gotrevor gotrevor commented Nov 1, 2016

This also addresses #165 , with a slightly different approach from #379.

Both db:migrate:status and db:migrate:pending would probably be useful.

@mention-bot
Copy link

@gotrevor, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sdepold, @Americas and @cusspvz to be potential reviewers.

return ensureCurrentMetaSchema(migrator).then(function () {
return migrator.executed();
}).then(function (migrations) {
_.forEach(migrations, function (migration) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like the way you are logging the results. Maybe something like this would be nicer:

Executed migrations:
- <migration file 1>
- <migration file 2>
...

Pending migrations:
- <migration file 3>
- <migration file 4>
...

If there are no executed or pending migrations, instead of that just log:

No executed migrations found.

No pending migrations found.

Copy link
Author

Choose a reason for hiding this comment

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

The format I chose was modeled after ActiveRecord's 'rake db:migrate:status', which will make it familiar to many users.
Having the 'up' or 'down' at the front of each line makes it easy to grep for any down migrations.
Adding the extra verbiage if there are no migrations of a particular type goes against the typical unix command-line style.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see your point here, it is helpful for chaining more commands on the output.

@Americas
Copy link
Collaborator

Americas commented Nov 3, 2016

I have written this in #379, I believe these two commands should exist as only one command, with optional arguments to tweak the result. WDYT of merging the functionalities of #379 with this command?

@gotrevor
Copy link
Author

gotrevor commented Nov 4, 2016

@Americas Either one of these PRs will be fine with me. I'm mostly interested in seeing down migrations.

@gotrevor
Copy link
Author

gotrevor commented Nov 4, 2016

ActiveRecord has db:migrate:status, very similar to this PR.
It has another command db:migrate: abort_if_pending_migrations, which is very similar to #379 :
https://github.com/rails/rails/blob/4-2-stable/activerecord/lib/active_record/railties/databases.rake#L165

Following their precedent of two separate commands, instead of one with options makes sense to me. It also makes for a nice parallel between the two migrations systems, which seems valuable.

@Americas
Copy link
Collaborator

Americas commented Nov 4, 2016

@sdepold @cusspvz opinions?

Should we create 2 commands or just add options to this one so it errors if there are pending migrations?

The rest LGTM

@gotrevor
Copy link
Author

gotrevor commented Nov 9, 2016

@Americas @sdepold @cusspvz
Based on this comment:
#379 (comment)
I think that this PR is ready to merge, and that one will be re-worked on top of it.

The question of "Should we create 2 commands or just add options to this one so it errors if there are pending migrations?" can be handled over there.

Can this get merged?

Thanks,
Trevor

@Americas Americas merged commit d38ebc8 into sequelize:master Nov 21, 2016
@gotrevor gotrevor mentioned this pull request Dec 6, 2016
@gotrevor gotrevor deleted the db-migrate-status branch December 30, 2016 19:04
codetriage-readme-bot pushed a commit to codetriage-readme-bot/cli that referenced this pull request Jun 5, 2019
Use go-update and semver for a better upgrade experience
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.

None yet

3 participants