From 7a109402a866db1c84ef9af1c14e148ec944aa22 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 20 Jan 2017 21:57:01 +0800
Subject: [PATCH] Prefer service object over after_save hook
Closes #26921
---
app/controllers/admin/runners_controller.rb | 6 +++---
.../projects/runners_controller.rb | 6 +++---
app/models/ci/runner.rb | 8 --------
app/services/ci/update_runner_service.rb | 15 ++++++++++++++
lib/ci/api/runners.rb | 20 +++++++++++--------
spec/models/ci/runner_spec.rb | 2 +-
.../services/ci/update_runner_service_spec.rb | 18 +++++++++++++++++
7 files changed, 52 insertions(+), 23 deletions(-)
create mode 100644 app/services/ci/update_runner_service.rb
create mode 100644 spec/services/ci/update_runner_service_spec.rb
diff --git a/app/controllers/admin/runners_controller.rb b/app/controllers/admin/runners_controller.rb
index 7345c91f67d..348641e5ecb 100644
--- a/app/controllers/admin/runners_controller.rb
+++ b/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.'
diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb
index 53c36635efe..ff75c408beb 100644
--- a/app/controllers/projects/runners_controller.rb
+++ b/app/controllers/projects/runners_controller.rb
@@ -16,7 +16,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'
@@ -32,7 +32,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.'
@@ -40,7 +40,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.'
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 6e58a1878c8..b4760b5baaa 100644
--- a/app/models/ci/runner.rb
+++ b/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)
@@ -149,12 +147,6 @@ module Ci
"runner:build_queue:#{self.token}"
end
- def form_editable_changed?
- FORM_EDITABLE.any? do |editable|
- public_send("#{editable}_changed?")
- end
- end
-
def tag_constraints
unless has_tags? || run_untagged?
errors.add(:tags_list,
diff --git a/app/services/ci/update_runner_service.rb b/app/services/ci/update_runner_service.rb
new file mode 100644
index 00000000000..198b29b3a9d
--- /dev/null
+++ b/app/services/ci/update_runner_service.rb
@@ -0,0 +1,15 @@
+module Ci
+ class UpdateRunnerService
+ attr_reader :runner
+
+ def initialize(runner)
+ @runner = runner
+ end
+
+ def update(params)
+ runner.update(params).tap do
+ runner.tick_runner_queue
+ end
+ end
+ end
+end
diff --git a/lib/ci/api/runners.rb b/lib/ci/api/runners.rb
index bcc82969eb3..c10858f4c5e 100644
--- a/lib/ci/api/runners.rb
+++ b/lib/ci/api/runners.rb
@@ -28,23 +28,27 @@ module Ci
post "register" do
required_attributes! [:token]
- attributes = attributes_for_keys(
- [:description, :tag_list, :run_untagged, :locked]
- )
-
+ project = nil
runner =
if runner_registration_token_valid?
# Create shared runner. Requires admin access
- Ci::Runner.create(attributes.merge(is_shared: true))
+ Ci::Runner.new(is_shared: true)
elsif project = Project.find_by(runners_token: params[:token])
- # Create a specific runner for project.
- project.runners.create(attributes)
+ Ci::Runner.new
end
return forbidden! unless runner
+ attributes = attributes_for_keys(
+ [:description, :tag_list, :run_untagged, :locked]
+ ).merge(get_runner_version_from_params || {})
+
+ Ci::UpdateRunnerService.new(runner).update(attributes)
+
+ # Assign the specific runner for the project
+ project.runners << runner if project
+
if runner.id
- runner.update(get_runner_version_from_params)
present runner, with: Entities::Runner
else
not_found!
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 2b856ca7af7..7b993a454b7 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -291,7 +291,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
diff --git a/spec/services/ci/update_runner_service_spec.rb b/spec/services/ci/update_runner_service_spec.rb
new file mode 100644
index 00000000000..8429881dd15
--- /dev/null
+++ b/spec/services/ci/update_runner_service_spec.rb
@@ -0,0 +1,18 @@
+require 'spec_helper'
+
+describe Ci::UpdateRunnerService, services: true do
+ let(:runner) { create(:ci_runner) }
+
+ describe '#update' do
+ before do
+ allow(runner).to receive(:tick_runner_queue)
+
+ described_class.new(runner).update(description: 'new runner')
+ end
+
+ it 'updates the runner and ticking the queue' do
+ expect(runner.description).to eq('new runner')
+ expect(runner).to have_received(:tick_runner_queue)
+ end
+ end
+end
--
GitLab