Merge branch 'fix-ci-runners-version-update' into 'master'

Fix CI runner version not being properly updated when asked 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.

See merge request !2618

7 files changed, 42 insertions, 6 deletions

diff --git a/CHANGELOG b/CHANGELOG
index e6fb2e9f508..e05095887e2 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -17,6 +17,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