Merge branch 'lefthook-outdated-todos' into 'master'

Allow Danger::OutdatedTodo to fail in Lefthook

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/136686



Merged-by: Bojan Marjanovic <bmarjanovic@gitlab.com>
Approved-by: Bojan Marjanovic <bmarjanovic@gitlab.com>
Approved-by: Alina Mihaila <amihaila@gitlab.com>
Approved-by: Alex Buijs <abuijs@gitlab.com>
Reviewed-by: Bojan Marjanovic <bmarjanovic@gitlab.com>
Co-authored-by: Dmitry Gruzd <dgruzd@gitlab.com>

danger/plugins/todos.rb

@@ -5,7 +5,11 @@
 module Danger
   class Todos < ::Danger::Plugin
     def check_outdated_todos(filenames)
-      Tooling::Danger::OutdatedTodo.new(filenames, context: self).check
+      Tooling::Danger::OutdatedTodo.new(filenames, context: self, allow_fail: from_lefthook?).check
+    end
+
+    def from_lefthook?
+      %w[1 true].include?(ENV['FROM_LEFTHOOK'])
     end
   end
 end

lefthook.yml

@@ -4,7 +4,9 @@ pre-push:
     - ref: master
   commands:
     danger:
-      run: bundle exec rake danger_local
+      files: git diff --name-only $(git merge-base origin/master HEAD)..HEAD
+      # We need to specify {files} as part of the command, otherwise it won't execute the hook
+      run: echo {files} >/dev/null && FROM_LEFTHOOK=1 bundle exec rake danger_local
     eslint:
       tags: frontend style
       files: git diff --name-only --diff-filter=d $(git merge-base origin/master HEAD)..HEAD

spec/tooling/danger/outdated_todo_spec.rb

@@ -15,69 +15,78 @@
     ]
   end
 
-  subject(:plugin) { described_class.new(filenames, context: fake_danger, todos: todos) }
-
-  context 'when the filenames are mentioned in single todo' do
-    let(:filenames) { ['app/controllers/acme_challenges_controller.rb'] }
-
-    it 'warns about mentions' do
-      expect(fake_danger)
-        .to receive(:warn)
-        .with <<~MESSAGE
-          `app/controllers/acme_challenges_controller.rb` was removed but is mentioned in:
-          - `spec/fixtures/tooling/danger/rubocop_todo/cop1.yml:5`
-        MESSAGE
-
-      plugin.check
-    end
-  end
-
-  context 'when the filenames are mentioned in multiple todos' do
-    let(:filenames) do
-      [
-        'app/controllers/application_controller.rb',
-        'app/controllers/acme_challenges_controller.rb'
-      ]
-    end
-
-    it 'warns about mentions' do
-      expect(fake_danger)
-        .to receive(:warn)
-        .with(<<~FIRSTMESSAGE)
-          `app/controllers/application_controller.rb` was removed but is mentioned in:
-          - `spec/fixtures/tooling/danger/rubocop_todo/cop1.yml:4`
-          - `spec/fixtures/tooling/danger/rubocop_todo/cop2.yml:4`
-        FIRSTMESSAGE
-
-      expect(fake_danger)
-        .to receive(:warn)
-        .with(<<~SECONDMESSAGE)
-          `app/controllers/acme_challenges_controller.rb` was removed but is mentioned in:
-          - `spec/fixtures/tooling/danger/rubocop_todo/cop1.yml:5`
-        SECONDMESSAGE
-
-      plugin.check
-    end
-  end
-
-  context 'when the filenames are not mentioned in todos' do
-    let(:filenames) { ['any/inexisting/file.rb'] }
-
-    it 'does not warn' do
-      expect(fake_danger).not_to receive(:warn)
-
-      plugin.check
-    end
-  end
-
-  context 'when there is no todos' do
-    let(:filenames) { ['app/controllers/acme_challenges_controller.rb'] }
-    let(:todos) { [] }
-
-    it 'does not warn' do
-      expect(fake_danger).not_to receive(:warn)
-
-      plugin.check
+  subject(:plugin) { described_class.new(filenames, context: fake_danger, todos: todos, allow_fail: allow_fail) }
+
+  [true, false].each do |allow_failure|
+    context "with allow_fail set to #{allow_failure}" do
+      let(:allow_fail) { allow_failure }
+      let(:expected_method) do
+        allow_failure ? :fail : :warn
+      end
+
+      context 'when the filenames are mentioned in single todo' do
+        let(:filenames) { ['app/controllers/acme_challenges_controller.rb'] }
+
+        it 'warns about mentions' do
+          expect(fake_danger)
+            .to receive(expected_method)
+            .with <<~MESSAGE
+              `app/controllers/acme_challenges_controller.rb` was removed but is mentioned in:
+              - `spec/fixtures/tooling/danger/rubocop_todo/cop1.yml:5`
+            MESSAGE
+
+          plugin.check
+        end
+      end
+
+      context 'when the filenames are mentioned in multiple todos' do
+        let(:filenames) do
+          [
+            'app/controllers/application_controller.rb',
+            'app/controllers/acme_challenges_controller.rb'
+          ]
+        end
+
+        it 'warns about mentions' do
+          expect(fake_danger)
+            .to receive(expected_method)
+            .with(<<~FIRSTMESSAGE)
+              `app/controllers/application_controller.rb` was removed but is mentioned in:
+              - `spec/fixtures/tooling/danger/rubocop_todo/cop1.yml:4`
+              - `spec/fixtures/tooling/danger/rubocop_todo/cop2.yml:4`
+            FIRSTMESSAGE
+
+          expect(fake_danger)
+            .to receive(expected_method)
+            .with(<<~SECONDMESSAGE)
+              `app/controllers/acme_challenges_controller.rb` was removed but is mentioned in:
+              - `spec/fixtures/tooling/danger/rubocop_todo/cop1.yml:5`
+            SECONDMESSAGE
+
+          plugin.check
+        end
+      end
+
+      context 'when the filenames are not mentioned in todos' do
+        let(:filenames) { ['any/inexisting/file.rb'] }
+
+        it 'does not warn' do
+          expect(fake_danger).not_to receive(expected_method)
+
+          plugin.check
+        end
+      end
+
+      context 'when there is no todos' do
+        let(:filenames) { ['app/controllers/acme_challenges_controller.rb'] }
+        let(:todos) { [] }
+
+        it 'does not warn' do
+          expect(fake_danger).not_to receive(expected_method)
+
+          plugin.check
+        end
+      end
     end
   end
 end

tooling/danger/outdated_todo.rb

@@ -8,10 +8,11 @@ class OutdatedTodo
         spec/support/rspec_order_todo.yml
       ].freeze
 
-      def initialize(filenames, context:, todos: TODOS_GLOBS)
+      def initialize(filenames, context:, todos: TODOS_GLOBS, allow_fail: false)
         @filenames = filenames
         @context = context
         @todos_globs = todos
+        @allow_fail = allow_fail
       end
 
       def check
@@ -22,17 +23,23 @@ def check
 
       private
 
-      attr_reader :filenames, :context
+      attr_reader :filenames, :context, :allow_fail
 
       def check_filename(filename)
         mentions = all_mentions_for(filename)
 
         return if mentions.empty?
 
-        context.warn <<~MESSAGE
+        message = <<~MESSAGE
           `#{filename}` was removed but is mentioned in:
           #{mentions.join("\n")}
         MESSAGE
+
+        if allow_fail
+          context.fail message
+        else
+          context.warn message
+        end
       end
 
       def all_mentions_for(filename)