From 95d2f0fb51de2c5800d41b7c4f285ed533ffe21a Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Tue, 26 Jan 2016 19:25:48 +0100
Subject: [PATCH] Fix CI runner version not being properly updated when asking
for a build
Due to broken implementation of attribute_for_keys the runner information was not updated correctly.
This MR adds test to check that such scenario will never happen again.
---
CHANGELOG | 1 +
lib/api/helpers.rb | 5 +++--
lib/ci/api/builds.rb | 2 +-
lib/ci/api/helpers.rb | 10 +++++++---
lib/ci/api/runners.rb | 1 +
spec/requests/ci/api/builds_spec.rb | 15 +++++++++++++++
spec/requests/ci/api/runners_spec.rb | 14 ++++++++++++++
7 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/CHANGELOG b/CHANGELOG
index 4b283f235a4..c2b43d32c48 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -16,6 +16,7 @@ v 8.4.2 (unreleased)
- Bump required gitlab-workhorse version to bring in a fix for missing
artifacts in the build artifacts browser
- Get rid of those ugly borders on the file tree view
+ - Fix updating the runner information when asking for builds
- Bump gitlab_git version to 7.2.24 in order to bring in a performance
improvement when checking if a repository was empty
- Add instrumentation for Gitlab::Git::Repository instance methods so we can
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 3f528b9f7c0..9dacf7c1e86 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -153,10 +153,11 @@ module API
end
def attributes_for_keys(keys, custom_params = nil)
+ params_hash = custom_params || params
attrs = {}
keys.each do |key|
- if params[key].present? or (params.has_key?(key) and params[key] == false)
- attrs[key] = params[key]
+ if params_hash[key].present? or (params_hash.has_key?(key) and params_hash[key] == false)
+ attrs[key] = params_hash[key]
end
end
ActionController::Parameters.new(attrs).permit!
diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb
index 690bbf97a89..416b0b5f0b4 100644
--- a/lib/ci/api/builds.rb
+++ b/lib/ci/api/builds.rb
@@ -13,13 +13,13 @@ module Ci
post "register" do
authenticate_runner!
update_runner_last_contact
+ update_runner_info
required_attributes! [:token]
not_found! unless current_runner.active?
build = Ci::RegisterBuildService.new.execute(current_runner)
if build
- update_runner_info
present build, with: Entities::BuildDetails
else
not_found!
diff --git a/lib/ci/api/helpers.rb b/lib/ci/api/helpers.rb
index 1c91204e98c..199d62d9b8a 100644
--- a/lib/ci/api/helpers.rb
+++ b/lib/ci/api/helpers.rb
@@ -34,10 +34,14 @@ module Ci
@runner ||= Runner.find_by_token(params[:token].to_s)
end
- def update_runner_info
+ def get_runner_version_from_params
return unless params["info"].present?
- info = attributes_for_keys(["name", "version", "revision", "platform", "architecture"], params["info"])
- current_runner.update(info)
+ attributes_for_keys(["name", "version", "revision", "platform", "architecture"], params["info"])
+ end
+
+ def update_runner_info
+ current_runner.assign_attributes(get_runner_version_from_params)
+ current_runner.save if current_runner.changed?
end
def max_artifacts_size
diff --git a/lib/ci/api/runners.rb b/lib/ci/api/runners.rb
index bfc14fe7a6b..192b1d18a51 100644
--- a/lib/ci/api/runners.rb
+++ b/lib/ci/api/runners.rb
@@ -47,6 +47,7 @@ module Ci
return forbidden! unless runner
if runner.id
+ runner.update(get_runner_version_from_params)
present runner, with: Entities::Runner
else
not_found!
diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb
index eec927102aa..244947762dd 100644
--- a/spec/requests/ci/api/builds_spec.rb
+++ b/spec/requests/ci/api/builds_spec.rb
@@ -113,6 +113,21 @@ describe Ci::API::API do
expect(json_response["depends_on_builds"].count).to eq(2)
expect(json_response["depends_on_builds"][0]["name"]).to eq("rspec")
end
+
+ %w(name version revision platform architecture).each do |param|
+ context "updates runner #{param}" do
+ let(:value) { "#{param}_value" }
+
+ subject { runner.read_attribute(param.to_sym) }
+
+ it do
+ post ci_api("/builds/register"), token: runner.token, info: { param => value }
+ expect(response.status).to eq(404)
+ runner.reload
+ is_expected.to eq(value)
+ end
+ end
+ end
end
describe "PUT /builds/:id" do
diff --git a/spec/requests/ci/api/runners_spec.rb b/spec/requests/ci/api/runners_spec.rb
index 5942aa7a1b5..db8189ffb79 100644
--- a/spec/requests/ci/api/runners_spec.rb
+++ b/spec/requests/ci/api/runners_spec.rb
@@ -51,6 +51,20 @@ describe Ci::API::API do
expect(response.status).to eq(400)
end
+
+ %w(name version revision platform architecture).each do |param|
+ context "creates runner with #{param} saved" do
+ let(:value) { "#{param}_value" }
+
+ subject { Ci::Runner.first.read_attribute(param.to_sym) }
+
+ it do
+ post ci_api("/runners/register"), token: registration_token, info: { param => value }
+ expect(response.status).to eq(201)
+ is_expected.to eq(value)
+ end
+ end
+ end
end
describe "DELETE /runners/delete" do
--
GitLab