-
Notifications
You must be signed in to change notification settings - Fork 49
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
Implements migration scripts #268
base: master
Are you sure you want to change the base?
Conversation
@@ -8,6 +8,8 @@ gem 'eventmachine' if ENV['EM'] | |||
gem "activesupport", "< 5.0.0" | |||
gem "activemodel", "< 5.0.0" | |||
|
|||
gem 'generator_spec' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be in the 'development' group? or we want it as a real dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understood the way it work, Gemfile
in a gem is just like add_development_dependency
and therefore shouldn't be a real dependency.
Only the one declared in the gemspec file with add_dependency
will be real dependency.
If that statement is wrong, then I should update it, of course!
raise unless respond_to?(:down) | ||
|
||
begin | ||
down |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Print an error message right before calling down with the exception, and the fact that you are running down. People need to know what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sure I will.
begin | ||
down | ||
raise | ||
rescue StandardError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem useful?
|
||
def rollback! | ||
down if respond_to?(:down) | ||
rescue StandardError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what's that rescue block gives you, can you explain?
# | ||
class Migrator | ||
def self.migrations_table_name | ||
'nobrainer_migration_versions' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a document class for the migration table, similar to lib/no_brainer/lock.rb
You can call it MigrationState, and the table can be called 'nobrainer_migrations'
This should simplify your table manipulation logic.
@@ -16,7 +16,8 @@ module NoBrainer | |||
# Code that is loaded through the DSL of NoBrainer should not be eager loaded. | |||
autoload :Document, :IndexManager, :Loader, :Fork, :Geo, :SymbolDecoration | |||
eager_autoload :Config, :Connection, :ConnectionManager, :Error, | |||
:QueryRunner, :Criteria, :RQL, :Lock, :ReentrantLock, :Profiler, :System | |||
:QueryRunner, :Criteria, :RQL, :Lock, :ReentrantLock, :Profiler, | |||
:System, :Migrator, :Migration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should they be eager loaded? and not just loaded on demand ?
@@ -37,5 +43,22 @@ namespace :nobrainer do | |||
task :reset => [:drop, :sync_schema_quiet, :seed] | |||
|
|||
task :create => [:sync_schema] | |||
task :migrate => [:sync_schema] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we sync_schema still?
@path = path | ||
@version, snake_class_name = path.scan(%r{^.*/(\d+)_(.*)\.rb}).flatten | ||
|
||
@name = snake_class_name ? snake_class_name.camelize : nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use try(:camelize)
def initialize(path) | ||
@path = path | ||
@version, snake_class_name = path.scan(%r{^.*/(\d+)_(.*)\.rb}).flatten | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
take out new line
This PR adds support for migration scripts with nobrainer, heavily inspired from the ActiveRecord Migration Script concept (meaning familiar to RoR people that we are).
Migration script is really helpful in the data migration concern. Fix wrongly created data, deleting old data and more, can be easily done with migration scripts.
It is especially true as I've implemented the Capistrano integration too, exactly as AR is doing.
I've added a very basic generator which generates a migration script file named the same way as AR does. The migration scripts are inheriting from the new
NoBrainer::Migration
class which holds the logic of runningup
thendown
ifup
failed.Finally the new
NoBrainer::Migrator
class contains the heart of this PR and take care of:Benchmark.measure
block the migration script and shows the execution time (I've stoled theannounce
method from ActiveRecord)The migrator supports both migrating all non executed scripts, and rollbacking the last script only.
So it's a very minimal implementation with tests everywhere AFAIK.
I'm using this branch for a production project at work and it works pretty well. I really hope you'll accept it 🙏. I'm of course ready to fix all the things that would need it.