Merge branch 'pedropombeiro/349540/add-unregister-audit-event' into 'master'

Extract runner destruction logic to UnregisterRunnerService

See merge request gitlab-org/gitlab!79668

app/controllers/admin/runners_controller.rb

@@ -34,7 +34,7 @@ class Admin::RunnersController < Admin::ApplicationController
   end
 
   def destroy
-    @runner.destroy
+    Ci::UnregisterRunnerService.new(@runner).execute
 
     redirect_to admin_runners_path, status: :found
   end

app/controllers/groups/runners_controller.rb

@@ -35,7 +35,7 @@ class Groups::RunnersController < Groups::ApplicationController
     if @runner.belongs_to_more_than_one_project?
       redirect_to group_settings_ci_cd_path(@group, anchor: 'runners-settings'), status: :found, alert: _('Runner was not deleted because it is assigned to multiple projects.')
     else
-      @runner.destroy
+      Ci::UnregisterRunnerService.new(@runner).execute
 
       redirect_to group_settings_ci_cd_path(@group, anchor: 'runners-settings'), status: :found
     end

app/controllers/projects/runners_controller.rb

@@ -23,7 +23,7 @@ class Projects::RunnersController < Projects::ApplicationController
 
   def destroy
     if @runner.only_for?(project)
-      @runner.destroy
+      Ci::UnregisterRunnerService.new(@runner).execute
     end
 
     redirect_to project_runners_path(@project), status: :found

app/graphql/mutations/ci/runner/delete.rb

@@ -20,7 +20,7 @@ module Mutations
           error = authenticate_delete_runner!(runner)
           return { errors: [error] } if error
 
-          runner.destroy!
+          ::Ci::UnregisterRunnerService.new(runner).execute
 
           { errors: runner.errors.full_messages }
         end

app/services/ci/unregister_runner_service.rb

+# frozen_string_literal: true
+
+module Ci
+  class UnregisterRunnerService
+    attr_reader :runner
+
+    # @param [Ci::Runner] runner the runner to unregister/destroy
+    def initialize(runner)
+      @runner = runner
+    end
+
+    def execute
+      @runner&.destroy
+    end
+  end
+end

lib/api/ci/runner.rb

@@ -57,7 +57,7 @@ module API
         delete '/', feature_category: :runner do
           authenticate_runner!
 
-          destroy_conditionally!(current_runner)
+          destroy_conditionally!(current_runner) { ::Ci::UnregisterRunnerService.new(current_runner).execute }
         end
 
         desc 'Validates authentication credentials' do

lib/api/ci/runners.rb

@@ -110,7 +110,7 @@ module API
 
           authenticate_delete_runner!(runner)
 
-          destroy_conditionally!(runner)
+          destroy_conditionally!(runner) { ::Ci::UnregisterRunnerService.new(runner).execute }
         end
 
         desc 'List jobs running on a runner' do

spec/controllers/admin/runners_controller_spec.rb

@@ -4,9 +4,10 @@ require 'spec_helper'
 
 RSpec.describe Admin::RunnersController do
   let_it_be(:runner) { create(:ci_runner) }
+  let_it_be(:user) { create(:admin) }
 
   before do
-    sign_in(create(:admin))
+    sign_in(user)
   end
 
   describe '#index' do
@@ -104,6 +105,10 @@ RSpec.describe Admin::RunnersController do
 
   describe '#destroy' do
     it 'destroys the runner' do
+      expect_next_instance_of(Ci::UnregisterRunnerService, runner) do |service|
+        expect(service).to receive(:execute).once.and_call_original
+      end
+
       delete :destroy, params: { id: runner.id }
 
       expect(response).to have_gitlab_http_status(:found)

spec/controllers/groups/runners_controller_spec.rb

@@ -190,6 +190,10 @@ RSpec.describe Groups::RunnersController do
       end
 
       it 'destroys the runner and redirects' do
+        expect_next_instance_of(Ci::UnregisterRunnerService, runner) do |service|
+          expect(service).to receive(:execute).once.and_call_original
+        end
+
         delete :destroy, params: params
 
         expect(response).to have_gitlab_http_status(:found)

spec/controllers/projects/runners_controller_spec.rb

@@ -37,6 +37,10 @@ RSpec.describe Projects::RunnersController do
 
   describe '#destroy' do
     it 'destroys the runner' do
+      expect_next_instance_of(Ci::UnregisterRunnerService, runner) do |service|
+        expect(service).to receive(:execute).once.and_call_original
+      end
+
       delete :destroy, params: params
 
       expect(response).to have_gitlab_http_status(:found)

spec/graphql/mutations/ci/runner/delete_spec.rb

@@ -11,9 +11,7 @@ RSpec.describe Mutations::Ci::Runner::Delete do
   let(:current_ctx) { { current_user: user } }
 
   let(:mutation_params) do
-    {
-      id: runner.to_global_id
-    }
+    { id: runner.to_global_id }
   end
 
   specify { expect(described_class).to require_graphql_authorizations(:delete_runner) }
@@ -57,6 +55,10 @@ RSpec.describe Mutations::Ci::Runner::Delete do
         it 'deletes runner' do
           mutation_params[:id] = project_runner.to_global_id
 
+          expect_next_instance_of(::Ci::UnregisterRunnerService, project_runner) do |service|
+            expect(service).to receive(:execute).once.and_call_original
+          end
+
           expect { subject }.to change { Ci::Runner.count }.by(-1)
           expect(subject[:errors]).to be_empty
         end
@@ -73,6 +75,9 @@ RSpec.describe Mutations::Ci::Runner::Delete do
         it 'does not delete project runner' do
           mutation_params[:id] = two_projects_runner.to_global_id
 
+          allow_next_instance_of(::Ci::UnregisterRunnerService) do |service|
+            expect(service).not_to receive(:execute).once
+          end
           expect { subject }.not_to change { Ci::Runner.count }
           expect(subject[:errors]).to contain_exactly("Runner #{two_projects_runner.to_global_id} associated with more than one project")
         end
@@ -84,6 +89,10 @@ RSpec.describe Mutations::Ci::Runner::Delete do
       let(:current_ctx) { { current_user: admin_user } }
 
       it 'deletes runner' do
+        expect_next_instance_of(::Ci::UnregisterRunnerService, runner) do |service|
+          expect(service).to receive(:execute).once.and_call_original
+        end
+
         expect { subject }.to change { Ci::Runner.count }.by(-1)
         expect(subject[:errors]).to be_empty
       end

spec/requests/api/ci/runners_spec.rb

@@ -530,6 +530,10 @@ RSpec.describe API::Ci::Runners do
     context 'admin user' do
       context 'when runner is shared' do
         it 'deletes runner' do
+          expect_next_instance_of(Ci::UnregisterRunnerService, shared_runner) do |service|
+            expect(service).to receive(:execute).once.and_call_original
+          end
+
           expect do
             delete api("/runners/#{shared_runner.id}", admin)
 
@@ -544,6 +548,10 @@ RSpec.describe API::Ci::Runners do
 
       context 'when runner is not shared' do
         it 'deletes used project runner' do
+          expect_next_instance_of(Ci::UnregisterRunnerService, project_runner) do |service|
+            expect(service).to receive(:execute).once.and_call_original
+          end
+
           expect do
             delete api("/runners/#{project_runner.id}", admin)
 
@@ -553,6 +561,10 @@ RSpec.describe API::Ci::Runners do
       end
 
       it 'returns 404 if runner does not exist' do
+        allow_next_instance_of(Ci::UnregisterRunnerService) do |service|
+          expect(service).not_to receive(:execute)
+        end
+
         delete api('/runners/0', admin)
 
         expect(response).to have_gitlab_http_status(:not_found)
@@ -634,6 +646,10 @@ RSpec.describe API::Ci::Runners do
 
     context 'unauthorized user' do
       it 'does not delete project runner' do
+        allow_next_instance_of(Ci::UnregisterRunnerService) do |service|
+          expect(service).not_to receive(:execute)
+        end
+
         delete api("/runners/#{project_runner.id}")
 
         expect(response).to have_gitlab_http_status(:unauthorized)

spec/services/ci/unregister_runner_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Ci::UnregisterRunnerService, '#execute' do
+  subject { described_class.new(runner).execute }
+
+  let(:runner) { create(:ci_runner) }
+
+  it 'destroys runner' do
+    expect(runner).to receive(:destroy).once.and_call_original
+    expect { subject }.to change { Ci::Runner.count }.by(-1)
+    expect(runner[:errors]).to be_nil
+  end
+end