Merge branch 'pedropombeiro/505810/pass-runner_manager' into 'master'

Pass runner_manager to destroy_conditionally! call

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



Merged-by: Stan Hu <stanhu@gmail.com>
Approved-by: Dillon Wheeler <dwheeler@gitlab.com>
Approved-by: Stan Hu <stanhu@gmail.com>
Reviewed-by: Pedro Pombeiro <noreply@pedro.pombei.ro>
Co-authored-by: Pedro Pombeiro <noreply@pedro.pombei.ro>

app/services/ci/runners/unregister_runner_manager_service.rb

@@ -28,7 +28,7 @@ def execute
       private
 
       def system_id_missing_error
-        ServiceResponse.error(message: '`system_id` needs to be specified for runners created in the UI.')
+        ServiceResponse.error(message: '`system_id` needs to be specified.')
       end
     end
   end

lib/api/ci/runner.rb

@@ -98,11 +98,15 @@ class Runner < ::API::Base
         delete '/managers', urgency: :low, feature_category: :fleet_visibility do
           authenticate_runner!(ensure_runner_manager: false)
 
-          destroy_conditionally!(current_runner) do
+          runner_manager = current_runner.runner_managers.find_by_system_xid(params[:system_id])
+          not_found!('Runner manager not found') unless runner_manager
+
+          destroy_conditionally!(runner_manager) do
             ::Ci::Runners::UnregisterRunnerManagerService.new(
               current_runner,
               params[:token],
-              system_id: params[:system_id]).execute
+              system_id: params[:system_id]
+            ).execute
           end
         end
 

spec/requests/api/ci/runner/runners_delete_spec.rb

@@ -102,75 +102,67 @@
 
       context 'with created runner' do
         let!(:runner) { create(:ci_runner, :with_runner_manager, registration_type: :authenticated_user) }
+        let(:delete_params) do
+          { token: runner.token, system_id: runner.runner_managers.first.system_xid }
+        end
 
-        context 'with matching system_id' do
-          context 'when no token is provided' do
-            let(:delete_params) { { system_id: runner.runner_managers.first.system_xid } }
-
-            it 'returns 400 error' do
-              delete_request
-
-              expect(response).to have_gitlab_http_status(:bad_request)
-            end
-          end
-
-          context 'when invalid token is provided' do
-            let(:delete_params) { { token: 'invalid', system_id: runner.runner_managers.first.system_xid } }
+        it 'deletes runner manager' do
+          expect do
+            delete_request
 
-            it 'returns 403 error' do
-              delete_request
+            expect(response).to have_gitlab_http_status(:no_content)
+          end.to change { runner.runner_managers.count }.from(1).to(0)
+            .and not_change { ::Ci::Runner.count }.from(1)
+        end
 
-              expect(response).to have_gitlab_http_status(:forbidden)
-            end
-          end
+        it_behaves_like '412 response' do
+          let(:request) { api('/runners/managers') }
+          let(:params) { delete_params }
         end
-      end
 
-      context 'when valid token is provided' do
-        context 'with created runner' do
-          let!(:runner) { create(:ci_runner, :with_runner_manager, registration_type: :authenticated_user) }
+        it_behaves_like 'storing arguments in the application context for the API' do
+          let(:expected_params) { { client_id: "runner/#{runner.id}" } }
+        end
 
-          context 'with matching system_id' do
-            let(:delete_params) { { token: runner.token, system_id: runner.runner_managers.first.system_xid } }
+        context 'with unknown system_id' do
+          let(:delete_params) { { token: runner.token, system_id: 'unknown_system_id' } }
 
-            it 'deletes runner manager' do
-              expect do
-                delete_request
+          it 'returns 404 error' do
+            delete_request
 
-                expect(response).to have_gitlab_http_status(:no_content)
-              end.to change { runner.runner_managers.count }.from(1).to(0)
+            expect(response).to have_gitlab_http_status(:not_found)
+            expect(response.body).to include('Runner manager not found')
+            expect(::Ci::Runner.count).to eq(1)
+          end
+        end
 
-              expect(::Ci::Runner.count).to eq(1)
-            end
+        context 'without system_id' do
+          let(:delete_params) { { token: runner.token } }
 
-            it_behaves_like '412 response' do
-              let(:request) { api('/runners/managers') }
-              let(:params) { delete_params }
-            end
+          it 'does not delete runner manager nor runner' do
+            delete_request
 
-            it_behaves_like 'storing arguments in the application context for the API' do
-              let(:expected_params) { { client_id: "runner/#{runner.id}" } }
-            end
+            expect(response).to have_gitlab_http_status(:bad_request)
           end
+        end
 
-          context 'with unknown system_id' do
-            let(:delete_params) { { token: runner.token, system_id: 'unknown_system_id' } }
+        context 'when no token is provided' do
+          let(:delete_params) { { system_id: runner.runner_managers.first.system_xid } }
 
-            it 'returns 404 error' do
-              delete_request
+          it 'returns 400 error' do
+            delete_request
 
-              expect(response).to have_gitlab_http_status(:not_found)
-            end
+            expect(response).to have_gitlab_http_status(:bad_request)
           end
+        end
 
-          context 'without system_id' do
-            let(:delete_params) { { token: runner.token } }
+        context 'when invalid token is provided' do
+          let(:delete_params) { { token: 'invalid', system_id: runner.runner_managers.first.system_xid } }
 
-            it 'does not delete runner manager nor runner' do
-              delete_request
+          it 'returns 403 error' do
+            delete_request
 
-              expect(response).to have_gitlab_http_status(:bad_request)
-            end
+            expect(response).to have_gitlab_http_status(:forbidden)
           end
         end
       end

spec/services/ci/runners/unregister_runner_manager_service_spec.rb

@@ -74,7 +74,7 @@
       it 'returns error and leaves runner_manager1', :aggregate_failures do
         expect do
           expect(execute).to be_error
-          expect(execute.message).to eq('`system_id` needs to be specified for runners created in the UI.')
+          expect(execute.message).to eq('`system_id` needs to be specified.')
         end.to not_change { Ci::Runner.count }
            .and not_change { Ci::RunnerManager.count }
       end