diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index ff8aa351226a1ac8a8e1171ed309fddfbb604aa2..f566dfd76e9bf8891b65d838df74125c18909d36 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -148,7 +148,7 @@ spinach 9 10: *spinach-knapsack
.spinach-knapsack-ruby23: &spinach-knapsack-ruby23
<<: *spinach-knapsack
<<: *ruby-23
-
+
rspec 0 20 ruby23: *rspec-knapsack-ruby23
rspec 1 20 ruby23: *rspec-knapsack-ruby23
rspec 2 20 ruby23: *rspec-knapsack-ruby23
@@ -196,6 +196,7 @@ rake flog: *exec
rake flay: *exec
rake db:migrate:reset: *exec
license_finder: *exec
+rake downtime_check: *exec
bundler:audit:
stage: test
diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md
index e2ca46504e71093dbf7ca7f824b7c6ac19537d8a..b8fab3aaff7e376d5cf20b3d4aa978a033182e45 100644
--- a/doc/development/migration_style_guide.md
+++ b/doc/development/migration_style_guide.md
@@ -11,7 +11,8 @@ migrations are written carefully, can be applied online and adhere to the style
Migrations should not require GitLab installations to be taken offline unless
_absolutely_ necessary. If a migration requires downtime this should be
clearly mentioned during the review process as well as being documented in the
-monthly release post.
+monthly release post. For more information see the "Downtime Tagging" section
+below.
When writing your migrations, also consider that databases might have stale data
or inconsistencies and guard for that. Try to make as little assumptions as possible
@@ -20,35 +21,34 @@ about the state of the database.
Please don't depend on GitLab specific code since it can change in future versions.
If needed copy-paste GitLab code into the migration to make it forward compatible.
-## Comments in the migration
+## Downtime Tagging
-Each migration you write needs to have the two following pieces of information
-as comments.
+Every migration must specify if it requires downtime or not, and if it should
+require downtime it must also specify a reason for this. To do so, add the
+following two constants to the migration class' body:
-### Online, Offline, errors?
+* `DOWNTIME`: a boolean that when set to `true` indicates the migration requires
+ downtime.
+* `DOWNTIME_REASON`: a String containing the reason for the migration requiring
+ downtime. This constant **must** be set when `DOWNTIME` is set to `true`.
-First, you need to provide information on whether the migration can be applied:
+For example:
-1. online without errors (works on previous version and new one)
-2. online with errors on old instances after migrating
-3. online with errors on new instances while migrating
-4. offline (needs to happen without app servers to prevent db corruption)
-
-For example:
-
-```
-# Migration type: online without errors (works on previous version and new one)
+```ruby
class MyMigration < ActiveRecord::Migration
-...
-```
+ DOWNTIME = true
+ DOWNTIME_REASON = 'This migration requires downtime because ...'
-It is always preferable to have a migration run online. If you expect the migration
-to take particularly long (for instance, if it loops through all notes),
-this is valuable information to add.
+ def change
+ ...
+ end
+end
+```
-If you don't provide the information it means that a migration is safe to run online.
+It is an error (that is, CI will fail) if the `DOWNTIME` constant is missing
+from a migration class.
-### Reversibility
+## Reversibility
Your migration should be reversible. This is very important, as it should
be possible to downgrade in case of a vulnerability or bugs.
@@ -100,7 +100,7 @@ value of `10` you'd write the following:
class MyMigration < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
-
+
def up
add_column_with_default(:projects, :foo, :integer, default: 10)
end
diff --git a/generator_templates/active_record/migration/create_table_migration.rb b/generator_templates/active_record/migration/create_table_migration.rb
index 27acc75dcc433d9980e92871bdc035af80044c3a..aad8626a7206346bbafac4a5a3bd7cfbc0d9fdfd 100644
--- a/generator_templates/active_record/migration/create_table_migration.rb
+++ b/generator_templates/active_record/migration/create_table_migration.rb
@@ -4,6 +4,14 @@
class <%= migration_class_name %> < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
+ # Set this constant to true if this migration requires downtime.
+ DOWNTIME = false
+
+ # When a migration requires downtime you **must** uncomment the following
+ # constant and define a short and easy to understand explanation as to why the
+ # migration requires downtime.
+ # DOWNTIME_REASON = ''
+
# When using the methods "add_concurrent_index" or "add_column_with_default"
# you must disable the use of transactions as these methods can not run in an
# existing transaction. When using "add_concurrent_index" make sure that this
diff --git a/generator_templates/active_record/migration/migration.rb b/generator_templates/active_record/migration/migration.rb
index 06bdea113670ab16b0370f222bd02ab5fc42b3d1..825bc8bdf6154c9aad05ba7571a21c247d84f0ba 100644
--- a/generator_templates/active_record/migration/migration.rb
+++ b/generator_templates/active_record/migration/migration.rb
@@ -4,6 +4,14 @@
class <%= migration_class_name %> < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
+ # Set this constant to true if this migration requires downtime.
+ DOWNTIME = false
+
+ # When a migration requires downtime you **must** uncomment the following
+ # constant and define a short and easy to understand explanation as to why the
+ # migration requires downtime.
+ # DOWNTIME_REASON = ''
+
# When using the methods "add_concurrent_index" or "add_column_with_default"
# you must disable the use of transactions as these methods can not run in an
# existing transaction. When using "add_concurrent_index" make sure that this
diff --git a/lib/gitlab/downtime_check.rb b/lib/gitlab/downtime_check.rb
new file mode 100644
index 0000000000000000000000000000000000000000..ab9537ed7d701752ff9c020da8a45448c016a65a
--- /dev/null
+++ b/lib/gitlab/downtime_check.rb
@@ -0,0 +1,71 @@
+module Gitlab
+ # Checks if a set of migrations requires downtime or not.
+ class DowntimeCheck
+ # The constant containing the boolean that indicates if downtime is needed
+ # or not.
+ DOWNTIME_CONST = :DOWNTIME
+
+ # The constant that specifies the reason for the migration requiring
+ # downtime.
+ DOWNTIME_REASON_CONST = :DOWNTIME_REASON
+
+ # Checks the given migration paths and returns an Array of
+ # `Gitlab::DowntimeCheck::Message` instances.
+ #
+ # migrations - The migration file paths to check.
+ def check(migrations)
+ migrations.map do |path|
+ require(path)
+
+ migration_class = class_for_migration_file(path)
+
+ unless migration_class.const_defined?(DOWNTIME_CONST)
+ raise "The migration in #{path} does not specify if it requires " \
+ "downtime or not"
+ end
+
+ if online?(migration_class)
+ Message.new(path)
+ else
+ reason = downtime_reason(migration_class)
+
+ unless reason
+ raise "The migration in #{path} requires downtime but no reason " \
+ "was given"
+ end
+
+ Message.new(path, true, reason)
+ end
+ end
+ end
+
+ # Checks the given migrations and prints the results to STDOUT/STDERR.
+ #
+ # migrations - The migration file paths to check.
+ def check_and_print(migrations)
+ check(migrations).each do |message|
+ puts message.to_s # rubocop: disable Rails/Output
+ end
+ end
+
+ # Returns the class for the given migration file path.
+ def class_for_migration_file(path)
+ File.basename(path, File.extname(path)).split('_', 2).last.camelize.
+ constantize
+ end
+
+ # Returns true if the given migration can be performed without downtime.
+ def online?(migration)
+ migration.const_get(DOWNTIME_CONST) == false
+ end
+
+ # Returns the downtime reason, or nil if none was defined.
+ def downtime_reason(migration)
+ if migration.const_defined?(DOWNTIME_REASON_CONST)
+ migration.const_get(DOWNTIME_REASON_CONST)
+ else
+ nil
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/downtime_check/message.rb b/lib/gitlab/downtime_check/message.rb
new file mode 100644
index 0000000000000000000000000000000000000000..4446e921e0df989d2cd0b7a54dafd84388becfe9
--- /dev/null
+++ b/lib/gitlab/downtime_check/message.rb
@@ -0,0 +1,28 @@
+module Gitlab
+ class DowntimeCheck
+ class Message
+ attr_reader :path, :offline, :reason
+
+ OFFLINE = "\e[32moffline\e[0m"
+ ONLINE = "\e[31monline\e[0m"
+
+ # path - The file path of the migration.
+ # offline - When set to `true` the migration will require downtime.
+ # reason - The reason as to why the migration requires downtime.
+ def initialize(path, offline = false, reason = nil)
+ @path = path
+ @offline = offline
+ @reason = reason
+ end
+
+ def to_s
+ label = offline ? OFFLINE : ONLINE
+
+ message = "[#{label}]: #{path}"
+ message += ": #{reason}" if reason
+
+ message
+ end
+ end
+ end
+end
diff --git a/lib/tasks/downtime_check.rake b/lib/tasks/downtime_check.rake
new file mode 100644
index 0000000000000000000000000000000000000000..30a2e9be5ce7415bf3b888e6461dcd5bfec374a7
--- /dev/null
+++ b/lib/tasks/downtime_check.rake
@@ -0,0 +1,26 @@
+desc 'Checks if migrations in a branch require downtime'
+task downtime_check: :environment do
+ # First we'll want to make sure we're comparing with the right upstream
+ # repository/branch.
+ current_branch = `git rev-parse --abbrev-ref HEAD`.strip
+
+ # Either the developer ran this task directly on the master branch, or they're
+ # making changes directly on the master branch.
+ if current_branch == 'master'
+ if defined?(Gitlab::License)
+ repo = 'gitlab-ee'
+ else
+ repo = 'gitlab-ce'
+ end
+
+ `git fetch https://gitlab.com/gitlab-org/#{repo}.git --depth 1`
+
+ compare_with = 'FETCH_HEAD'
+ # The developer is working on a different branch, in this case we can just
+ # compare with the master branch.
+ else
+ compare_with = 'master'
+ end
+
+ Rake::Task['gitlab:db:downtime_check'].invoke(compare_with)
+end
diff --git a/lib/tasks/gitlab/db.rake b/lib/tasks/gitlab/db.rake
index 7230b9485bea741e5d64c6429321c680adb12fbd..0ec19e1a62501ea30aad04f13f80d6de4499abec 100644
--- a/lib/tasks/gitlab/db.rake
+++ b/lib/tasks/gitlab/db.rake
@@ -46,5 +46,20 @@ namespace :gitlab do
Rake::Task['db:seed_fu'].invoke
end
end
+
+ desc 'Checks if migrations require downtime or not'
+ task :downtime_check, [:ref] => :environment do |_, args|
+ abort 'You must specify a Git reference to compare with' unless args[:ref]
+
+ require 'shellwords'
+
+ ref = Shellwords.escape(args[:ref])
+
+ migrations = `git diff #{ref}.. --name-only -- db/migrate`.lines.
+ map { |file| Rails.root.join(file.strip).to_s }.
+ select { |file| File.file?(file) }
+
+ Gitlab::DowntimeCheck.new.check_and_print(migrations)
+ end
end
end
diff --git a/spec/lib/gitlab/downtime_check/message_spec.rb b/spec/lib/gitlab/downtime_check/message_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..93094cda776f03f3e7b31a629c8b59f20192fc5d
--- /dev/null
+++ b/spec/lib/gitlab/downtime_check/message_spec.rb
@@ -0,0 +1,17 @@
+require 'spec_helper'
+
+describe Gitlab::DowntimeCheck::Message do
+ describe '#to_s' do
+ it 'returns an ANSI formatted String for an offline migration' do
+ message = described_class.new('foo.rb', true, 'hello')
+
+ expect(message.to_s).to eq("[\e[32moffline\e[0m]: foo.rb: hello")
+ end
+
+ it 'returns an ANSI formatted String for an online migration' do
+ message = described_class.new('foo.rb')
+
+ expect(message.to_s).to eq("[\e[31monline\e[0m]: foo.rb")
+ end
+ end
+end
diff --git a/spec/lib/gitlab/downtime_check_spec.rb b/spec/lib/gitlab/downtime_check_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..42d895e548e3aa97b948034d83a8148f46c77f4e
--- /dev/null
+++ b/spec/lib/gitlab/downtime_check_spec.rb
@@ -0,0 +1,113 @@
+require 'spec_helper'
+
+describe Gitlab::DowntimeCheck do
+ subject { described_class.new }
+ let(:path) { 'foo.rb' }
+
+ describe '#check' do
+ before do
+ expect(subject).to receive(:require).with(path)
+ end
+
+ context 'when a migration does not specify if downtime is required' do
+ it 'raises RuntimeError' do
+ expect(subject).to receive(:class_for_migration_file).
+ with(path).
+ and_return(Class.new)
+
+ expect { subject.check([path]) }.
+ to raise_error(RuntimeError, /it requires downtime/)
+ end
+ end
+
+ context 'when a migration requires downtime' do
+ context 'when no reason is specified' do
+ it 'raises RuntimeError' do
+ stub_const('TestMigration::DOWNTIME', true)
+
+ expect(subject).to receive(:class_for_migration_file).
+ with(path).
+ and_return(TestMigration)
+
+ expect { subject.check([path]) }.
+ to raise_error(RuntimeError, /no reason was given/)
+ end
+ end
+
+ context 'when a reason is specified' do
+ it 'returns an Array of messages' do
+ stub_const('TestMigration::DOWNTIME', true)
+ stub_const('TestMigration::DOWNTIME_REASON', 'foo')
+
+ expect(subject).to receive(:class_for_migration_file).
+ with(path).
+ and_return(TestMigration)
+
+ messages = subject.check([path])
+
+ expect(messages).to be_an_instance_of(Array)
+ expect(messages[0]).to be_an_instance_of(Gitlab::DowntimeCheck::Message)
+
+ message = messages[0]
+
+ expect(message.path).to eq(path)
+ expect(message.offline).to eq(true)
+ expect(message.reason).to eq('foo')
+ end
+ end
+ end
+ end
+
+ describe '#check_and_print' do
+ it 'checks the migrations and prints the results to STDOUT' do
+ stub_const('TestMigration::DOWNTIME', true)
+ stub_const('TestMigration::DOWNTIME_REASON', 'foo')
+
+ expect(subject).to receive(:require).with(path)
+
+ expect(subject).to receive(:class_for_migration_file).
+ with(path).
+ and_return(TestMigration)
+
+ expect(subject).to receive(:puts).with(an_instance_of(String))
+
+ subject.check_and_print([path])
+ end
+ end
+
+ describe '#class_for_migration_file' do
+ it 'returns the class for a migration file path' do
+ expect(subject.class_for_migration_file('123_string.rb')).to eq(String)
+ end
+ end
+
+ describe '#online?' do
+ it 'returns true when a migration can be performed online' do
+ stub_const('TestMigration::DOWNTIME', false)
+
+ expect(subject.online?(TestMigration)).to eq(true)
+ end
+
+ it 'returns false when a migration can not be performed online' do
+ stub_const('TestMigration::DOWNTIME', true)
+
+ expect(subject.online?(TestMigration)).to eq(false)
+ end
+ end
+
+ describe '#downtime_reason' do
+ context 'when a reason is defined' do
+ it 'returns the downtime reason' do
+ stub_const('TestMigration::DOWNTIME_REASON', 'hello')
+
+ expect(subject.downtime_reason(TestMigration)).to eq('hello')
+ end
+ end
+
+ context 'when a reason is not defined' do
+ it 'returns nil' do
+ expect(subject.downtime_reason(Class.new)).to be_nil
+ end
+ end
+ end
+end