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

Move db:migrate:status to DatabaseTasks method #34081

Merged
merged 1 commit into from Oct 9, 2018

Conversation

@gmcgibbon
Copy link
Member

@gmcgibbon gmcgibbon commented Oct 4, 2018

Summary

Move db:migrate:status code to ActiveRecord::Tasks::DatabaseTasks method. I've also changed the abort call to an actual exception. This made multi-db statuses easier to implement so I'm PR-ing this first. We gain some test coverage for migration status too!

r? @eileencodes
/cc @rafaelfranca

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:db_migrate_status_move branch from d476858 to ee071d8 Oct 4, 2018
Copy link
Contributor

@albertoalmagro albertoalmagro left a comment

Hi @gmcgibbon !

Nice refactor! 👏Thanks also for adding extra test coverage to this rake task 👍 I leave you just one inline comment regarding style, please tell me what do you think.

Thanks!

@@ -812,6 +814,34 @@ def capture_migration_output
end
end
end

class DatabaseTasksMigrateStatusTest < DatabaseTasksMigrationTestCase
test "migrate status table" do

This comment has been minimized.

@albertoalmagro

albertoalmagro Oct 4, 2018
Contributor

I know this varies from file to file, but I think it is better to maintain the same style as in the other tests, at least in the same file, I would therefore declare this as:

def test_migrate_status_table

The same applies to:

def migrate_status_table_without_migrations_table

This comment has been minimized.

@gmcgibbon

gmcgibbon Oct 5, 2018
Author Member

My mistake, I noticed they were using setup do..end and teardown do..end, so I assumed they were also using test "desc" do..end as well. Thanks for pointing that out 👍

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:db_migrate_status_move branch 2 times, most recently from 864aac2 to e92abaf Oct 5, 2018
Copy link
Contributor

@albertoalmagro albertoalmagro left a comment

Thanks for changing this! ❤️ I have seen that test_migrate_status_table_without_migrations_table was gone with the rebase. Did you notice that?

@eileencodes
Copy link
Member

@eileencodes eileencodes commented Oct 5, 2018

I'm not sure if it's failed before in the same way or whether this failure is new, but SQLite3 on 2.5.1 is failing with "Schema migrations table does not exist yet."

https://travis-ci.org/rails/rails/jobs/437454850

Otherwise this looks good 🎉

@gmcgibbon
Copy link
Member Author

@gmcgibbon gmcgibbon commented Oct 5, 2018

@albertoalmagro Yes, I removed it because there was already test coverage for rails db:migrate:status at the rake task level that was testing for an abort here.

@eileencodes Sorry, it seems on CI the migrations table doesn't get created automatically whereas locally it does. Invoking the functions to create and drop the migrations table seemed to work last CI run so I'll try that again.

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:db_migrate_status_move branch 4 times, most recently from 96bc4d0 to b04f062 Oct 5, 2018
Copy link
Contributor

@albertoalmagro albertoalmagro left a comment

Build is already OK, one job's build seems to be stalled. +1! Thanks ❤️

Copy link
Member

@eileencodes eileencodes left a comment

Sorry I missed the changelog entry before. Can you remove it and then I'll merge.

@@ -1,3 +1,7 @@
* Move `db:migrate:status` code to `ActiveRecord::Tasks::DatabaseTasks.migrate_status`.

*Gannon McGibbon*

This comment has been minimized.

@eileencodes

eileencodes Oct 7, 2018
Member

Can you remove this changleog entry? I don't think it's necessary (rake task acts the same) and I think this may make users think that they can't use db:migrate:status anymore.

This comment has been minimized.

@gmcgibbon

gmcgibbon Oct 8, 2018
Author Member

👍 Removed!

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:db_migrate_status_move branch from b04f062 to 0d435c1 Oct 8, 2018
Copy link
Member

@rafaelfranca rafaelfranca left a comment

:shipit:

@eileencodes eileencodes merged commit 1ddfee9 into rails:master Oct 9, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gmcgibbon gmcgibbon deleted the gmcgibbon:db_migrate_status_move branch Oct 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants