summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@gitlab.com>2017-12-11 16:34:51 +0000
committerSean McGivern <sean@gitlab.com>2017-12-11 16:34:51 +0000
commit1ab33b15d1f1cc6ce69514e545da1348fd750771 (patch)
treeaf4dab71355a5199facc7937aeb03f964e2f6d0c
parent8ff63039f1ee5f6e31a8b910e323977e7de3c634 (diff)
downloadgitlab-ce-1ab33b15d1f1cc6ce69514e545da1348fd750771.tar.gz
Add cop for use of remove_columnadd-remove-column-cop
remove_column should only be used in the up (or change) step of a migration if it's a post-deployment migration. Otherwise there will be downtime due to the ActiveRecord column cache, which we can avoid by using the IgnorableColumn concern in combination with a post-deployment migration.
-rw-r--r--db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb1
-rw-r--r--db/migrate/20160610301627_remove_notification_level_from_users.rb1
-rw-r--r--db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb1
-rw-r--r--db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb1
-rw-r--r--db/migrate/20160729173930_remove_project_id_from_spam_logs.rb1
-rw-r--r--db/migrate/20160831223750_remove_features_enabled_from_projects.rb1
-rw-r--r--db/migrate/20160913162434_remove_projects_pushes_since_gc.rb1
-rw-r--r--db/migrate/20161018024550_remove_priority_from_labels.rb1
-rw-r--r--db/migrate/20161201160452_migrate_project_statistics.rb1
-rw-r--r--db/migrate/20170222143500_remove_old_project_id_columns.rb1
-rw-r--r--db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb1
-rw-r--r--db/migrate/20170315174634_revert_add_notified_of_own_activity_to_users.rb1
-rw-r--r--db/migrate/20171106154015_remove_issues_branch_name.rb1
-rw-r--r--rubocop/cop/migration/remove_column.rb30
-rw-r--r--rubocop/migration_helpers.rb6
-rw-r--r--rubocop/rubocop.rb1
-rw-r--r--spec/rubocop/cop/migration/remove_column_spec.rb68
17 files changed, 118 insertions, 0 deletions
diff --git a/db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb b/db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb
index 477b2106dea..21b367711c3 100644
--- a/db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb
+++ b/db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
class RemoveDeprecatedIssuesTrackerColumnsFromProjects < ActiveRecord::Migration
def change
remove_column :projects, :issues_tracker, :string, default: 'gitlab', null: false
diff --git a/db/migrate/20160610301627_remove_notification_level_from_users.rb b/db/migrate/20160610301627_remove_notification_level_from_users.rb
index 8afb14df2cf..356e53b4b23 100644
--- a/db/migrate/20160610301627_remove_notification_level_from_users.rb
+++ b/db/migrate/20160610301627_remove_notification_level_from_users.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
class RemoveNotificationLevelFromUsers < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
diff --git a/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
index 52a9819c628..058bd539e65 100644
--- a/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
+++ b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
diff --git a/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
index 4a7bde7f9f3..d0e5da4d28b 100644
--- a/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
+++ b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
diff --git a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb
index e28ab31d629..baf254c3bcc 100644
--- a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb
+++ b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
diff --git a/db/migrate/20160831223750_remove_features_enabled_from_projects.rb b/db/migrate/20160831223750_remove_features_enabled_from_projects.rb
index aec709aaf59..9eafd8b9477 100644
--- a/db/migrate/20160831223750_remove_features_enabled_from_projects.rb
+++ b/db/migrate/20160831223750_remove_features_enabled_from_projects.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
diff --git a/db/migrate/20160913162434_remove_projects_pushes_since_gc.rb b/db/migrate/20160913162434_remove_projects_pushes_since_gc.rb
index df7d922b816..f32167037e0 100644
--- a/db/migrate/20160913162434_remove_projects_pushes_since_gc.rb
+++ b/db/migrate/20160913162434_remove_projects_pushes_since_gc.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
diff --git a/db/migrate/20161018024550_remove_priority_from_labels.rb b/db/migrate/20161018024550_remove_priority_from_labels.rb
index b7416cca664..bc25a43526c 100644
--- a/db/migrate/20161018024550_remove_priority_from_labels.rb
+++ b/db/migrate/20161018024550_remove_priority_from_labels.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
class RemovePriorityFromLabels < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
diff --git a/db/migrate/20161201160452_migrate_project_statistics.rb b/db/migrate/20161201160452_migrate_project_statistics.rb
index 82fbdf02444..a547409aaa5 100644
--- a/db/migrate/20161201160452_migrate_project_statistics.rb
+++ b/db/migrate/20161201160452_migrate_project_statistics.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
class MigrateProjectStatistics < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
diff --git a/db/migrate/20170222143500_remove_old_project_id_columns.rb b/db/migrate/20170222143500_remove_old_project_id_columns.rb
index 268144a2552..9bed38a3444 100644
--- a/db/migrate/20170222143500_remove_old_project_id_columns.rb
+++ b/db/migrate/20170222143500_remove_old_project_id_columns.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
# rubocop:disable RemoveIndex
class RemoveOldProjectIdColumns < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
diff --git a/db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb b/db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb
index 1a77d5934a3..0535c2ddaf2 100644
--- a/db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb
+++ b/db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
# rubocop:disable Migration/Datetime
class RemoveUnusedCiTablesAndColumns < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
diff --git a/db/migrate/20170315174634_revert_add_notified_of_own_activity_to_users.rb b/db/migrate/20170315174634_revert_add_notified_of_own_activity_to_users.rb
index 807dfcb385d..9b9098d115d 100644
--- a/db/migrate/20170315174634_revert_add_notified_of_own_activity_to_users.rb
+++ b/db/migrate/20170315174634_revert_add_notified_of_own_activity_to_users.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
# rubocop:disable Migration/UpdateLargeTable
class RevertAddNotifiedOfOwnActivityToUsers < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
diff --git a/db/migrate/20171106154015_remove_issues_branch_name.rb b/db/migrate/20171106154015_remove_issues_branch_name.rb
index 3d08225c96d..162b6bafab4 100644
--- a/db/migrate/20171106154015_remove_issues_branch_name.rb
+++ b/db/migrate/20171106154015_remove_issues_branch_name.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/RemoveColumn
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
diff --git a/rubocop/cop/migration/remove_column.rb b/rubocop/cop/migration/remove_column.rb
new file mode 100644
index 00000000000..e53eb2e07b2
--- /dev/null
+++ b/rubocop/cop/migration/remove_column.rb
@@ -0,0 +1,30 @@
+require_relative '../../migration_helpers'
+
+module RuboCop
+ module Cop
+ module Migration
+ # Cop that checks if remove_column is used in a regular (not
+ # post-deployment) migration.
+ class RemoveColumn < RuboCop::Cop::Cop
+ include MigrationHelpers
+
+ MSG = '`remove_column` must only be used in post-deployment migrations'.freeze
+
+ def on_def(node)
+ def_method = node.children[0]
+
+ return unless in_migration?(node) && !in_post_deployment_migration?(node)
+ return unless def_method == :change || def_method == :up
+
+ node.each_descendant(:send) do |send_node|
+ send_method = send_node.children[1]
+
+ if send_method == :remove_column
+ add_offense(send_node, :selector)
+ end
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/rubocop/migration_helpers.rb b/rubocop/migration_helpers.rb
index c3473771178..c066d424437 100644
--- a/rubocop/migration_helpers.rb
+++ b/rubocop/migration_helpers.rb
@@ -7,5 +7,11 @@ module RuboCop
dirname.end_with?('db/migrate', 'db/post_migrate')
end
+
+ def in_post_deployment_migration?(node)
+ dirname = File.dirname(node.location.expression.source_buffer.name)
+
+ dirname.end_with?('db/post_migrate')
+ end
end
end
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index 7621ea50da9..eb52be3d731 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -14,6 +14,7 @@ require_relative 'cop/migration/add_index'
require_relative 'cop/migration/add_timestamps'
require_relative 'cop/migration/datetime'
require_relative 'cop/migration/hash_index'
+require_relative 'cop/migration/remove_column'
require_relative 'cop/migration/remove_concurrent_index'
require_relative 'cop/migration/remove_index'
require_relative 'cop/migration/reversible_add_column_with_default'
diff --git a/spec/rubocop/cop/migration/remove_column_spec.rb b/spec/rubocop/cop/migration/remove_column_spec.rb
new file mode 100644
index 00000000000..89112f01723
--- /dev/null
+++ b/spec/rubocop/cop/migration/remove_column_spec.rb
@@ -0,0 +1,68 @@
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/remove_column'
+
+describe RuboCop::Cop::Migration::RemoveColumn do
+ include CopHelper
+
+ subject(:cop) { described_class.new }
+
+ def source(meth = 'change')
+ "def #{meth}; remove_column :table, :column; end"
+ end
+
+ context 'in a regular migration' do
+ before do
+ allow(cop).to receive(:in_migration?).and_return(true)
+ allow(cop).to receive(:in_post_deployment_migration?).and_return(false)
+ end
+
+ it 'registers an offense when remove_column is used in the change method' do
+ inspect_source(cop, source('change'))
+
+ aggregate_failures do
+ expect(cop.offenses.size).to eq(1)
+ expect(cop.offenses.map(&:line)).to eq([1])
+ end
+ end
+
+ it 'registers an offense when remove_column is used in the up method' do
+ inspect_source(cop, source('up'))
+
+ aggregate_failures do
+ expect(cop.offenses.size).to eq(1)
+ expect(cop.offenses.map(&:line)).to eq([1])
+ end
+ end
+
+ it 'registers no offense when remove_column is used in the down method' do
+ inspect_source(cop, source('down'))
+
+ expect(cop.offenses.size).to eq(0)
+ end
+ end
+
+ context 'in a post-deployment migration' do
+ before do
+ allow(cop).to receive(:in_migration?).and_return(true)
+ allow(cop).to receive(:in_post_deployment_migration?).and_return(true)
+ end
+
+ it 'registers no offense' do
+ inspect_source(cop, source)
+
+ expect(cop.offenses.size).to eq(0)
+ end
+ end
+
+ context 'outside of a migration' do
+ it 'registers no offense' do
+ inspect_source(cop, source)
+
+ expect(cop.offenses.size).to eq(0)
+ end
+ end
+end