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

Implements migration scripts #268

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zedtux
Copy link
Collaborator

@zedtux zedtux commented Nov 25, 2020

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 running up then down if up failed.

Finally the new NoBrainer::Migrator class contains the heart of this PR and take care of:

  • creating a table to store the script versions (well its timestamp, like AR does) to run the script only once
  • run in a Benchmark.measure block the migration script and shows the execution time (I've stoled the announce method from ActiveRecord)
  • save or delete the script version

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.

@@ -8,6 +8,8 @@ gem 'eventmachine' if ENV['EM']
gem "activesupport", "< 5.0.0"
gem "activemodel", "< 5.0.0"

gem 'generator_spec'
Copy link
Collaborator

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?

Copy link
Collaborator Author

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!

gemfiles/rails6.gemfile Show resolved Hide resolved
raise unless respond_to?(:down)

begin
down
Copy link
Collaborator

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.

Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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'
Copy link
Collaborator

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
Copy link
Collaborator

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]
Copy link
Collaborator

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
Copy link
Collaborator

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

take out new line

@zedtux zedtux self-assigned this Jul 10, 2021
@zedtux zedtux added the feature New functionalities label Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants