Extend rake task for obsolete column ignores

This extends `db:obsolete_ignored_columns` to also consider the removal
information given when ignoring the column. This basically respects the
`remove_after` date given with `ignore_column`.

app/models/concerns/ignorable_columns.rb

@@ -3,19 +3,32 @@
 module IgnorableColumns
   extend ActiveSupport::Concern
 
+  ColumnIgnore = Struct.new(:remove_after, :remove_with) do
+    def safe_to_remove?
+      Date.today > remove_after
+    end
+
+    def to_s
+      "(#{remove_after}, #{remove_with})"
+    end
+  end
+
   class_methods do
     # Ignore database columns in a model
     #
     # Indicate the earliest date and release we can stop ignoring the column with +remove_after+ (a date string) and +remove_with+ (a release)
     def ignore_columns(*columns, remove_after:, remove_with:)
-      raise ArgumentError, 'Please indicate when we can stop ignoring columns with remove_after, example: ignore_columns(:name, remove_after: \'2019-12-01\', remove_with: \'12.6\')' unless remove_after && remove_with
+      raise ArgumentError, 'Please indicate when we can stop ignoring columns with remove_after (date string), example: ignore_columns(:name, remove_after: \'2019-12-01\', remove_with: \'12.6\')' unless remove_after
+      raise ArgumentError, 'Please indicate in which release we can stop ignoring columns with remove_with, example: ignore_columns(:name, remove_after: \'2019-12-01\', remove_with: \'12.6\')' unless remove_with
 
       self.ignored_columns += columns.flatten
 
-      @ignored_columns_details ||= {}
+      unless defined?(@ignored_columns_details)
+        @ignored_columns_details = superclass.try(:ignored_columns_details)&.dup || {}
+      end
 
       columns.flatten.each do |column|
-        @ignored_columns_details[column] = { remove_after: remove_after, remove_with: remove_with }
+        @ignored_columns_details[column.to_sym] = ColumnIgnore.new(Date.parse(remove_after), remove_with)
       end
     end
 

lib/gitlab/database/obsolete_ignored_columns.rb

@@ -23,8 +23,15 @@ module Gitlab
       private
 
       def ignored_columns_safe_to_remove_for(klass)
-        ignored = klass.ignored_columns.map(&:to_s)
+        ignores = ignored_and_not_present(klass).each_with_object({}) do |col, h|
+          h[col] = klass.ignored_columns_details[col.to_sym]
+        end
+
+        ignores.select { |_, i| i&.safe_to_remove? }
+      end
 
+      def ignored_and_not_present(klass)
+        ignored = klass.ignored_columns.map(&:to_s)
         return [] if ignored.empty?
 
         schema = klass.connection.schema_cache.columns_hash(klass.table_name)

lib/tasks/db_obsolete_ignored_columns.rake

@@ -8,7 +8,10 @@ task 'db:obsolete_ignored_columns' => :environment do
     puts 'The following `ignored_columns` are obsolete and can be removed:'
 
     list.each do |name, ignored_columns|
-      puts "- #{name}: #{ignored_columns.join(', ')}"
+      puts "#{name}:"
+      ignored_columns.each do |column, removal|
+        puts " - #{column.ljust(30)} Remove after #{removal.remove_after} with #{removal.remove_with}"
+      end
     end
 
     puts <<~TEXT

spec/lib/gitlab/database/obsolete_ignored_columns_spec.rb

@@ -8,21 +8,27 @@ describe Gitlab::Database::ObsoleteIgnoredColumns do
     end
 
     class SomeAbstract < MyBase
+      include IgnorableColumns
+
       self.abstract_class = true
 
       self.table_name = 'projects'
 
-      self.ignored_columns += %i[unused]
+      ignore_column :unused, remove_after: '2019-01-01', remove_with: '12.0'
     end
 
     class B < MyBase
+      include IgnorableColumns
+
       self.table_name = 'issues'
 
-      self.ignored_columns += %i[id other]
+      ignore_column :id, :other, remove_after: '2019-01-01', remove_with: '12.0'
+      ignore_column :not_used_but_still_ignored, remove_after: Date.today.to_s, remove_with: '12.1'
     end
 
     class A < SomeAbstract
-      self.ignored_columns += %i[id also_unused]
+      ignore_column :also_unused, remove_after: '2019-02-01', remove_with: '12.1'
+      ignore_column :not_used_but_still_ignored, remove_after: Date.today.to_s, remove_with: '12.1'
     end
 
     class C < MyBase
@@ -35,8 +41,13 @@ describe Gitlab::Database::ObsoleteIgnoredColumns do
   describe '#execute' do
     it 'returns a list of class names and columns pairs' do
       expect(subject.execute).to eq([
-        ['Testing::A', %w(unused also_unused)],
-        ['Testing::B', %w(other)]
+        ['Testing::A', {
+          'unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0'),
+          'also_unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-02-01'), '12.1')
+        }],
+        ['Testing::B', {
+          'other' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0')
+        }]
       ])
     end
   end

spec/models/concerns/ignorable_columns_spec.rb

@@ -32,21 +32,53 @@ describe IgnorableColumns do
   end
 
   describe '.ignored_columns_details' do
-    it 'stores removal information' do
-      subject.ignore_column(:name, remove_after: '2019-12-01', remove_with: '12.6')
+    shared_examples_for 'storing removal information' do
+      it 'storing removal information' do
+        subject.ignore_column(columns, remove_after: '2019-12-01', remove_with: '12.6')
 
-      expect(subject.ignored_columns_details[:name]).to eq(remove_after: '2019-12-01', remove_with: '12.6')
+        [columns].flatten.each do |column|
+          expect(subject.ignored_columns_details[column].remove_after).to eq(Date.parse('2019-12-01'))
+          expect(subject.ignored_columns_details[column].remove_with).to eq('12.6')
+        end
+      end
     end
 
-    it 'stores removal information (array version)' do
-      subject.ignore_column(%i[name created_at], remove_after: '2019-12-01', remove_with: '12.6')
+    context 'with single column' do
+      let(:columns) { :name }
+      it_behaves_like 'storing removal information'
+    end
 
-      expect(subject.ignored_columns_details[:name]).to eq(remove_after: '2019-12-01', remove_with: '12.6')
-      expect(subject.ignored_columns_details[:created_at]).to eq(remove_after: '2019-12-01', remove_with: '12.6')
+    context 'with array column' do
+      let(:columns) { %i[name created_at] }
+      it_behaves_like 'storing removal information'
     end
 
     it 'defaults to empty Hash' do
       expect(subject.ignored_columns_details).to eq({})
     end
   end
+
+  describe IgnorableColumns::ColumnIgnore do
+    subject { described_class.new(remove_after, remove_with) }
+
+    let(:remove_with) { double }
+
+    describe '#safe_to_remove?' do
+      context 'after remove_after date has passed' do
+        let(:remove_after) { Date.parse('2019-01-10') }
+
+        it 'returns true (safe to remove)' do
+          expect(subject.safe_to_remove?).to be_truthy
+        end
+      end
+
+      context 'before remove_after date has passed' do
+        let(:remove_after) { Date.today }
+
+        it 'returns false (not safe to remove)' do
+          expect(subject.safe_to_remove?).to be_falsey
+        end
+      end
+    end
+  end
 end