From 3eafffcef0f8932bab4e06b465f9b63327e87942 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 16 Feb 2017 01:05:44 +0100 Subject: [PATCH] Refactor JobRequest response structure --- app/models/ci/build.rb | 29 +++++ ...ild_service.rb => register_job_service.rb} | 2 +- lib/api/entities.rb | 102 ++++++++++++------ lib/api/runner.rb | 6 +- lib/ci/api/builds.rb | 2 +- lib/gitlab/ci/build/response/image.rb | 20 ++++ lib/gitlab/ci/build/response/step.rb | 46 ++++++++ spec/factories/ci/builds.rb | 26 ++++- spec/requests/api/runner_spec.rb | 51 +++++++-- ...e_spec.rb => register_job_service_spec.rb} | 2 +- 10 files changed, 237 insertions(+), 49 deletions(-) rename app/services/ci/{register_build_service.rb => register_job_service.rb} (99%) create mode 100644 lib/gitlab/ci/build/response/image.rb create mode 100644 lib/gitlab/ci/build/response/step.rb rename spec/services/ci/{register_build_service_spec.rb => register_job_service_spec.rb} (99%) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index f2989eff22d..0c0bbb9d41d 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -508,6 +508,35 @@ module Ci ] end + def steps + [ + Gitlab::Ci::Build::Response::Step.from_commands(self), + Gitlab::Ci::Build::Response::Step.from_after_script(self) + ].compact + end + + def image + image = Gitlab::Ci::Build::Response::Image.new(options[:image]) + return unless image.valid? + image + end + + def services + services = options[:services].map do |service| + Gitlab::Ci::Build::Response::Image.new(service) + end + + services.select(&:valid?).compact + end + + def artifacts + options[:artifacts] + end + + def cache + options[:cache] + end + def credentials Gitlab::Ci::Build::Credentials::Factory.new(self).create! end diff --git a/app/services/ci/register_build_service.rb b/app/services/ci/register_job_service.rb similarity index 99% rename from app/services/ci/register_build_service.rb rename to app/services/ci/register_job_service.rb index 5b52a0425de..0ab9042bf24 100644 --- a/app/services/ci/register_build_service.rb +++ b/app/services/ci/register_job_service.rb @@ -1,7 +1,7 @@ module Ci # This class responsible for assigning # proper pending build to runner on runner API request - class RegisterBuildService + class RegisterJobService include Gitlab::CurrentSettings attr_reader :runner diff --git a/lib/api/entities.rb b/lib/api/entities.rb index cb3b409b8d0..7e07154600a 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -698,42 +698,82 @@ module API expose :active?, as: :active end - class ArtifactFile < Grape::Entity - expose :filename, :size - end - - class JobCredentials < Grape::Entity - expose :type, :url, :username, :password - end - - class JobResponse < Grape::Entity - expose :id, :ref, :tag, :sha, :status - expose :name, :token, :stage - expose :project_id - expose :project_name - expose :artifacts_file, using: ArtifactFile, if: ->(build, _) { build.artifacts? } - end - - class RequestJobResponse < JobResponse - expose :commands - expose :repo_url - expose :before_sha - expose :allow_git_fetch - expose :token - expose :artifacts_expire_at, if: ->(build, _) { build.artifacts? } - - expose :options do |model| - model.options + module JobRequest + class JobInfo < Grape::Entity + expose :name, :stage + expose :project_id, :project_name end - expose :timeout do |model| - model.timeout + class GitInfo < Grape::Entity + expose :repo_url, :ref, :sha, :before_sha + expose :ref_type do |model| + if model.tag + 'tag' + else + 'branch' + end + end end - expose :variables - expose :depends_on_builds, using: JobResponse + class RunnerInfo < Grape::Entity + expose :timeout + end - expose :credentials, using: JobCredentials + class Step < Grape::Entity + expose :name, :script, :timeout, :condition, :result + end + + class Image < Grape::Entity + expose :name + end + + class Artifacts < Grape::Entity + expose :name, :untracked, :paths, :when, :expire_in + end + + class Cache < Grape::Entity + expose :key, :untracked, :paths + end + + class Credentials < Grape::Entity + expose :type, :url, :username, :password + end + + class ArtifactFile < Grape::Entity + expose :filename, :size + end + + class Dependency < Grape::Entity + expose :id, :name + expose :artifacts_file, using: ArtifactFile, if: ->(job, _) { job.artifacts? } + end + + class Response < Grape::Entity + expose :id + expose :token + expose :allow_git_fetch + + expose :job_info, using: JobInfo do |model| + model + end + + expose :git_info, using: GitInfo do |model| + model + end + + expose :runner_info, using: RunnerInfo do |model| + model + end + + expose :variables + expose :steps, using: Step + expose :image, using: Image + expose :services, using: Image + expose :artifacts, using: Artifacts + expose :cache, using: Cache + expose :credentials, using: Credentials + expose :depends_on_builds, as: :dependencies, using: Dependency + end end end end diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 2e7b96e5169..8372b794739 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -51,7 +51,7 @@ module API resource :jobs do desc 'Request a job' do - success Entities::RequestJobResponse + success Entities::JobRequest::Response end params do requires :token, type: String, desc: %q(Runner's authentication token) @@ -68,13 +68,13 @@ module API end new_update = current_runner.ensure_runner_queue_value - result = ::Ci::RegisterBuildService.new(current_runner).execute + result = ::Ci::RegisterJobService.new(current_runner).execute if result.valid? if result.build Gitlab::Metrics.add_event(:build_found, project: result.build.project.path_with_namespace) - present result.build, with: Entities::RequestJobResponse + present result.build, with: Entities::JobRequest::Response else Gitlab::Metrics.add_event(:build_not_found) header 'X-GitLab-Last-Update', new_update diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index b51e76d93f2..746e76a1b1f 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -24,7 +24,7 @@ module Ci new_update = current_runner.ensure_runner_queue_value - result = Ci::RegisterBuildService.new(current_runner).execute + result = Ci::RegisterJobService.new(current_runner).execute if result.valid? if result.build diff --git a/lib/gitlab/ci/build/response/image.rb b/lib/gitlab/ci/build/response/image.rb new file mode 100644 index 00000000000..342a249aee8 --- /dev/null +++ b/lib/gitlab/ci/build/response/image.rb @@ -0,0 +1,20 @@ +module Gitlab + module Ci + module Build + module Response + class Image + attr_reader :name + + def initialize(image) + type = image.class + @name = image if type == String + end + + def valid? + @name != nil + end + end + end + end + end +end \ No newline at end of file diff --git a/lib/gitlab/ci/build/response/step.rb b/lib/gitlab/ci/build/response/step.rb new file mode 100644 index 00000000000..7b35852a510 --- /dev/null +++ b/lib/gitlab/ci/build/response/step.rb @@ -0,0 +1,46 @@ +module Gitlab + module Ci + module Build + module Response + class Step + CONDITION_IF_SUCCEEDED = 'run_if_succeeded' + CONDITION_ALWAYS = 'run_always' + + RESULT_FAILS_JOB = 'fails_job' + RESULT_DOESNT_FAIL_JOB = 'doesnt_fail_job' + + attr_reader :name, :script, :condition, :result, :timeout + + class << self + def from_commands(build) + self.new(:script, + build.commands, + build.timeout, + CONDITION_IF_SUCCEEDED, + RESULT_FAILS_JOB) + end + + def from_after_script(build) + after_script = build.options[:after_script] + return unless after_script + + self.new(:after_script, + after_script, + build.timeout, + CONDITION_ALWAYS, + RESULT_DOESNT_FAIL_JOB) + end + end + + def initialize(name, script, timeout, condition = CONDITION_IF_SUCCEEDED, result = RESULT_FAILS_JOB) + @name = name + @script = script.split("\n") + @timeout = timeout + @condition = condition + @result = result + end + end + end + end + end +end \ No newline at end of file diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index a90534d10ba..a77b3356b9a 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -15,8 +15,8 @@ FactoryGirl.define do options do { - image: "ruby:2.1", - services: ["postgres"] + image: 'ruby:2.1', + services: ['postgres'] } end @@ -151,5 +151,27 @@ FactoryGirl.define do allow(build).to receive(:commit).and_return build(:commit) end end + + trait :extended_options do + options do + { + image: 'ruby:2.1', + services: ['postgres'], + after_script: "ls\ndate", + artifacts: { + name: 'artifacts_file', + untracked: false, + paths: ['out/'], + when: 'always', + expire_in: '7d' + }, + cache: { + key: 'cache_key', + untracked: false, + paths: ['vendor/*'] + } + } + end + end end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 3b94be55974..bd9dc422703 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -152,8 +152,8 @@ describe API::Runner do describe '/api/v4/jobs' do let(:project) { create(:empty_project, shared_runners_enabled: false) } let(:pipeline) { create(:ci_pipeline_without_jobs, project: project, ref: 'master') } - let!(:job) { create(:ci_build, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) } let(:runner) { create(:ci_runner) } + let!(:job) { create(:ci_build, :artifacts, :extended_options, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0, commands: "ls\ndate") } before { project.runners << runner } @@ -271,14 +271,44 @@ describe API::Runner do expect(response).to have_http_status(201) expect(response.headers).not_to have_key('X-GitLab-Last-Update') - expect(json_response['sha']).to eq(job.sha) - expect(json_response['options']).to eq({'image' => 'ruby:2.1', 'services' => ['postgres']}) - expect(json_response['variables']).to include( - {'key' => 'CI_BUILD_NAME', 'value' => 'spinach', 'public' => true}, - {'key' => 'CI_BUILD_STAGE', 'value' => 'test', 'public' => true}, - {'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true} - ) expect(runner.reload.platform).to eq('darwin') + + expect(json_response['id']).to eq(job.id) + expect(json_response['token']).to eq(job.token) + expect(json_response['job_info']).to include({ 'name' => job.name }, + { 'stage' => job.stage }) + expect(json_response['git_info']).to include({ 'sha' => job.sha }, + { 'repo_url' => job.repo_url }) + expect(json_response['image']).to include({ 'name' => 'ruby:2.1' }) + expect(json_response['services']).to include({ 'name' => 'postgres' }) + expect(json_response['steps']).to include({ 'name' => 'after_script', + 'script' => ['ls', 'date'], + 'timeout' => job.timeout, + 'condition' => Gitlab::Ci::Build::Response::Step::CONDITION_ALWAYS, + 'result' => Gitlab::Ci::Build::Response::Step::RESULT_DOESNT_FAIL_JOB }) + expect(json_response['variables']).to include({ 'key' => 'CI_BUILD_NAME', 'value' => 'spinach', 'public' => true }, + { 'key' => 'CI_BUILD_STAGE', 'value' => 'test', 'public' => true }, + { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true }) + expect(json_response['artifacts']).to include({ 'name' => 'artifacts_file' }, + { 'paths' => ['out/'] }) + end + + context 'when job is made for tag' do + let!(:job) { create(:ci_build_tag, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) } + + it 'sets branch as ref_type' do + request_job + expect(response).to have_http_status(201) + expect(json_response['git_info']['ref_type']).to eq('tag') + end + end + + context 'when job is made for branch' do + it 'sets tag as ref_type' do + request_job + expect(response).to have_http_status(201) + expect(json_response['git_info']['ref_type']).to eq('branch') + end end it 'updates runner info' do @@ -322,8 +352,8 @@ describe API::Runner do expect(response).to have_http_status(201) expect(json_response['id']).to eq(test_job.id) - expect(json_response['depends_on_builds'].count).to eq(1) - expect(json_response['depends_on_builds'][0]).to include('id' => job.id, 'name' => 'spinach') + expect(json_response['dependencies'].count).to eq(1) + expect(json_response['dependencies'][0]).to include('id' => job.id, 'name' => 'spinach') end end @@ -381,6 +411,7 @@ describe API::Runner do it 'sends registry credentials key' do request_job + expect(json_response).to have_key('credentials') expect(json_response['credentials']).to include(registry_credentials) end diff --git a/spec/services/ci/register_build_service_spec.rb b/spec/services/ci/register_job_service_spec.rb similarity index 99% rename from spec/services/ci/register_build_service_spec.rb rename to spec/services/ci/register_job_service_spec.rb index cd7dd53025c..ea8b996d481 100644 --- a/spec/services/ci/register_build_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' module Ci - describe RegisterBuildService, services: true do + describe RegisterJobService, services: true do let!(:project) { FactoryGirl.create :empty_project, shared_runners_enabled: false } let!(:pipeline) { FactoryGirl.create :ci_pipeline, project: project } let!(:pending_build) { FactoryGirl.create :ci_build, pipeline: pipeline }