Merge branch 'use-update-runner-service' into 'master'

Prefer service object over after_save hook

Closes #26921

See merge request !8664

app/controllers/admin/runners_controller.rb

@@ -13,7 +13,7 @@ class Admin::RunnersController < Admin::ApplicationController
   end
 
   def update
-    if @runner.update_attributes(runner_params)
+    if Ci::UpdateRunnerService.new(@runner).update(runner_params)
       respond_to do |format|
         format.js
         format.html { redirect_to admin_runner_path(@runner) }
@@ -31,7 +31,7 @@ class Admin::RunnersController < Admin::ApplicationController
   end
 
   def resume
-    if @runner.update_attributes(active: true)
+    if Ci::UpdateRunnerService.new(@runner).update(active: true)
       redirect_to admin_runners_path, notice: 'Runner was successfully updated.'
     else
       redirect_to admin_runners_path, alert: 'Runner was not updated.'
@@ -39,7 +39,7 @@ class Admin::RunnersController < Admin::ApplicationController
   end
 
   def pause
-    if @runner.update_attributes(active: false)
+    if Ci::UpdateRunnerService.new(@runner).update(active: false)
       redirect_to admin_runners_path, notice: 'Runner was successfully updated.'
     else
       redirect_to admin_runners_path, alert: 'Runner was not updated.'

app/controllers/projects/runners_controller.rb

@@ -12,7 +12,7 @@ class Projects::RunnersController < Projects::ApplicationController
   end
 
   def update
-    if @runner.update_attributes(runner_params)
+    if Ci::UpdateRunnerService.new(@runner).update(runner_params)
       redirect_to runner_path(@runner), notice: 'Runner was successfully updated.'
     else
       render 'edit'
@@ -28,7 +28,7 @@ class Projects::RunnersController < Projects::ApplicationController
   end
 
   def resume
-    if @runner.update_attributes(active: true)
+    if Ci::UpdateRunnerService.new(@runner).update(active: true)
       redirect_to runner_path(@runner), notice: 'Runner was successfully updated.'
     else
       redirect_to runner_path(@runner), alert: 'Runner was not updated.'
@@ -36,7 +36,7 @@ class Projects::RunnersController < Projects::ApplicationController
   end
 
   def pause
-    if @runner.update_attributes(active: false)
+    if Ci::UpdateRunnerService.new(@runner).update(active: false)
       redirect_to runner_path(@runner), notice: 'Runner was successfully updated.'
     else
       redirect_to runner_path(@runner), alert: 'Runner was not updated.'

app/models/ci/runner.rb

@@ -22,8 +22,6 @@ module Ci
     scope :online, ->() { where('contacted_at > ?', LAST_CONTACT_TIME) }
     scope :ordered, ->() { order(id: :desc) }
 
-    after_save :tick_runner_queue, if: :form_editable_changed?
-
     scope :owned_or_shared, ->(project_id) do
       joins('LEFT JOIN ci_runner_projects ON ci_runner_projects.runner_id = ci_runners.id')
         .where("ci_runner_projects.gl_project_id = :project_id OR ci_runners.is_shared = true", project_id: project_id)
@@ -40,6 +38,8 @@ module Ci
 
     acts_as_taggable
 
+    after_destroy :cleanup_runner_queue
+
     # Searches for runners matching the given query.
     #
     # This method uses ILIKE on PostgreSQL and LIKE on MySQL.
@@ -147,14 +147,14 @@ module Ci
 
     private
 
-    def runner_queue_key
-      "runner:build_queue:#{self.token}"
+    def cleanup_runner_queue
+      Gitlab::Redis.with do |redis|
+        redis.del(runner_queue_key)
       end
-
-    def form_editable_changed?
-      FORM_EDITABLE.any? do |editable|
-        public_send("#{editable}_changed?")
     end
+
+    def runner_queue_key
+      "runner:build_queue:#{self.token}"
     end
 
     def tag_constraints

app/services/ci/update_runner_service.rb

+module Ci
+  class UpdateRunnerService
+    attr_reader :runner
+
+    def initialize(runner)
+      @runner = runner
+    end
+
+    def update(params)
+      runner.update(params).tap do |updated|
+        runner.tick_runner_queue if updated
+      end
+    end
+  end
+end

lib/api/runners.rb

@@ -60,8 +60,9 @@ module API
       put ':id' do
         runner = get_runner(params.delete(:id))
         authenticate_update_runner!(runner)
+        update_service = Ci::UpdateRunnerService.new(runner)
 
-        if runner.update(declared_params(include_missing: false))
+        if update_service.update(declared_params(include_missing: false))
           present runner, with: Entities::RunnerDetails, current_user: current_user
         else
           render_validation_error!(runner)

spec/controllers/admin/runners_controller_spec.rb

+require 'spec_helper'
+
+describe Admin::RunnersController do
+  let(:runner) { create(:ci_runner) }
+
+  before do
+    sign_in(create(:admin))
+  end
+
+  describe '#index' do
+    it 'lists all runners' do
+      get :index
+
+      expect(response).to have_http_status(200)
+    end
+  end
+
+  describe '#show' do
+    it 'shows a particular runner' do
+      get :show, id: runner.id
+
+      expect(response).to have_http_status(200)
+    end
+
+    it 'shows 404 for unknown runner' do
+      get :show, id: 0
+
+      expect(response).to have_http_status(404)
+    end
+  end
+
+  describe '#update' do
+    it 'updates the runner and ticks the queue' do
+      new_desc = runner.description.swapcase
+
+      expect do
+        post :update, id: runner.id, runner: { description: new_desc }
+      end.to change { runner.ensure_runner_queue_value }
+
+      runner.reload
+
+      expect(response).to have_http_status(302)
+      expect(runner.description).to eq(new_desc)
+    end
+  end
+
+  describe '#destroy' do
+    it 'destroys the runner' do
+      delete :destroy, id: runner.id
+
+      expect(response).to have_http_status(302)
+      expect(Ci::Runner.find_by(id: runner.id)).to be_nil
+    end
+  end
+
+  describe '#resume' do
+    it 'marks the runner as active and ticks the queue' do
+      runner.update(active: false)
+
+      expect do
+        post :resume, id: runner.id
+      end.to change { runner.ensure_runner_queue_value }
+
+      runner.reload
+
+      expect(response).to have_http_status(302)
+      expect(runner.active).to eq(true)
+    end
+  end
+
+  describe '#pause' do
+    it 'marks the runner as inactive and ticks the queue' do
+      runner.update(active: true)
+
+      expect do
+        post :pause, id: runner.id
+      end.to change { runner.ensure_runner_queue_value }
+
+      runner.reload
+
+      expect(response).to have_http_status(302)
+      expect(runner.active).to eq(false)
+    end
+  end
+end

spec/controllers/projects/runners_controller_spec.rb

+require 'spec_helper'
+
+describe Projects::RunnersController do
+  let(:user) { create(:user) }
+  let(:project) { create(:empty_project) }
+  let(:runner) { create(:ci_runner) }
+
+  let(:params) do
+    {
+      namespace_id: project.namespace,
+      project_id: project,
+      id: runner
+    }
+  end
+
+  before do
+    sign_in(user)
+    project.add_master(user)
+    project.runners << runner
+  end
+
+  describe '#update' do
+    it 'updates the runner and ticks the queue' do
+      new_desc = runner.description.swapcase
+
+      expect do
+        post :update, params.merge(runner: { description: new_desc } )
+      end.to change { runner.ensure_runner_queue_value }
+
+      runner.reload
+
+      expect(response).to have_http_status(302)
+      expect(runner.description).to eq(new_desc)
+    end
+  end
+
+  describe '#destroy' do
+    it 'destroys the runner' do
+      delete :destroy, params
+
+      expect(response).to have_http_status(302)
+      expect(Ci::Runner.find_by(id: runner.id)).to be_nil
+    end
+  end
+
+  describe '#resume' do
+    it 'marks the runner as active and ticks the queue' do
+      runner.update(active: false)
+
+      expect do
+        post :resume, params
+      end.to change { runner.ensure_runner_queue_value }
+
+      runner.reload
+
+      expect(response).to have_http_status(302)
+      expect(runner.active).to eq(true)
+    end
+  end
+
+  describe '#pause' do
+    it 'marks the runner as inactive and ticks the queue' do
+      runner.update(active: true)
+
+      expect do
+        post :pause, params
+      end.to change { runner.ensure_runner_queue_value }
+
+      runner.reload
+
+      expect(response).to have_http_status(302)
+      expect(runner.active).to eq(false)
+    end
+  end
+end

spec/models/ci/runner_spec.rb

@@ -290,7 +290,7 @@ describe Ci::Runner, models: true do
       let!(:last_update) { runner.ensure_runner_queue_value }
 
       before do
-        runner.update(description: 'new runner')
+        Ci::UpdateRunnerService.new(runner).update(description: 'new runner')
       end
 
       it 'sets a new last_update value' do
@@ -318,6 +318,25 @@ describe Ci::Runner, models: true do
     end
   end
 
+  describe '#destroy' do
+    let(:runner) { create(:ci_runner) }
+
+    context 'when there is a tick in the queue' do
+      let!(:queue_key) { runner.send(:runner_queue_key) }
+
+      before do
+        runner.tick_runner_queue
+        runner.destroy
+      end
+
+      it 'cleans up the queue' do
+        Gitlab::Redis.with do |redis|
+          expect(redis.get(queue_key)).to be_nil
+        end
+      end
+    end
+  end
+
   describe '.assignable_for' do
     let(:runner) { create(:ci_runner) }
     let(:project) { create(:empty_project) }

spec/requests/api/runners_spec.rb

@@ -183,6 +183,7 @@ describe API::Runners, api: true  do
         it 'updates runner' do
           description = shared_runner.description
           active = shared_runner.active
+          runner_queue_value = shared_runner.ensure_runner_queue_value
 
           update_runner(shared_runner.id, admin, description: "#{description}_updated",
                                                  active: !active,
@@ -197,18 +198,24 @@ describe API::Runners, api: true  do
           expect(shared_runner.tag_list).to include('ruby2.1', 'pgsql', 'mysql')
           expect(shared_runner.run_untagged?).to be(false)
           expect(shared_runner.locked?).to be(true)
+          expect(shared_runner.ensure_runner_queue_value)
+            .not_to eq(runner_queue_value)
         end
       end
 
       context 'when runner is not shared' do
         it 'updates runner' do
           description = specific_runner.description
+          runner_queue_value = specific_runner.ensure_runner_queue_value
+
           update_runner(specific_runner.id, admin, description: 'test')
           specific_runner.reload
 
           expect(response).to have_http_status(200)
           expect(specific_runner.description).to eq('test')
           expect(specific_runner.description).not_to eq(description)
+          expect(specific_runner.ensure_runner_queue_value)
+            .not_to eq(runner_queue_value)
         end
       end
 

spec/services/ci/update_runner_service_spec.rb

+require 'spec_helper'
+
+describe Ci::UpdateRunnerService, :services do
+  let(:runner) { create(:ci_runner) }
+
+  describe '#update' do
+    before do
+      allow(runner).to receive(:tick_runner_queue)
+    end
+
+    context 'with description params' do
+      let(:params) { { description: 'new runner' } }
+
+      it 'updates the runner and ticking the queue' do
+        expect(update).to be_truthy
+
+        runner.reload
+
+        expect(runner).to have_received(:tick_runner_queue)
+        expect(runner.description).to eq('new runner')
+      end
+    end
+
+    context 'when params are not valid' do
+      let(:params) { { run_untagged: false } }
+
+      it 'does not update and give false because it is not valid' do
+        expect(update).to be_falsey
+
+        runner.reload
+
+        expect(runner).not_to have_received(:tick_runner_queue)
+        expect(runner.run_untagged).to be_truthy
+      end
+    end
+
+    def update
+      described_class.new(runner).update(params)
+    end
+  end
+end