From 5c1aa5fb65ec7474956e6972e40b04b3a967c338 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 2 Mar 2017 17:44:15 +0100 Subject: [PATCH] Add some fixes and refactoring after review --- app/models/ci/build.rb | 14 +-- lib/api/runner.rb | 3 +- lib/gitlab/ci/build/image.rb | 34 ++++++ lib/gitlab/ci/build/response/image.rb | 20 ---- lib/gitlab/ci/build/response/step.rb | 44 -------- lib/gitlab/ci/build/step.rb | 56 ++++++++++ spec/factories/ci/builds.rb | 4 + spec/lib/gitlab/ci/build/image_spec.rb | 52 +++++++++ spec/lib/gitlab/ci/build/step_spec.rb | 33 ++++++ spec/requests/api/runner_spec.rb | 147 ++++++++++++++++++------- 10 files changed, 292 insertions(+), 115 deletions(-) create mode 100644 lib/gitlab/ci/build/image.rb delete mode 100644 lib/gitlab/ci/build/response/image.rb delete mode 100644 lib/gitlab/ci/build/response/step.rb create mode 100644 lib/gitlab/ci/build/step.rb create mode 100644 spec/lib/gitlab/ci/build/image_spec.rb create mode 100644 spec/lib/gitlab/ci/build/step_spec.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 0c0bbb9d41d..5ef089c87c5 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -510,23 +510,17 @@ module Ci def steps [ - Gitlab::Ci::Build::Response::Step.from_commands(self), - Gitlab::Ci::Build::Response::Step.from_after_script(self) + Gitlab::Ci::Build::Step.from_commands(self), + Gitlab::Ci::Build::Step.from_after_script(self) ].compact end def image - image = Gitlab::Ci::Build::Response::Image.new(options[:image]) - return unless image.valid? - image + Gitlab::Ci::Build::Image.from_image(self) end def services - services = options[:services].map do |service| - Gitlab::Ci::Build::Response::Image.new(service) - end - - services.select(&:valid?).compact + Gitlab::Ci::Build::Image.from_services(self) end def artifacts diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 23db32c9b1f..caa330c7234 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -176,6 +176,7 @@ module API end desc 'Upload artifacts for job' do + success Entities::JobRequest::Response http_codes [[201, 'Artifact uploaded'], [400, 'Bad request'], [403, 'Forbidden'], @@ -186,7 +187,7 @@ module API requires :id, type: Integer, desc: %q(Job's ID) optional :token, type: String, desc: %q(Job's authentication token) optional :expire_in, type: String, desc: %q(Specify when artifacts should expire) - optional 'file', type: File, desc: %q(Artifact's file) + optional :file, type: File, desc: %q(Artifact's file) optional 'file.path', type: String, desc: %q(path to locally stored body (generated by Workhorse)) optional 'file.name', type: String, desc: %q(real filename as send in Content-Disposition (generated by Workhorse)) optional 'file.type', type: String, desc: %q(real content type as send in Content-Type (generated by Workhorse)) diff --git a/lib/gitlab/ci/build/image.rb b/lib/gitlab/ci/build/image.rb new file mode 100644 index 00000000000..c0663f1ae8a --- /dev/null +++ b/lib/gitlab/ci/build/image.rb @@ -0,0 +1,34 @@ +module Gitlab + module Ci + module Build + class Image + attr_reader :name + + class << self + def from_image(job) + image = Gitlab::Ci::Build::Image.new(job.options[:image]) + return unless image.valid? + image + end + + def from_services(job) + services = job.options[:services].to_a.map do |service| + Gitlab::Ci::Build::Image.new(service) + end + + services.select(&:valid?).compact + end + end + + def initialize(image) + type = image.class + @name = image if type == String + end + + def valid? + @name.present? + end + end + end + end +end diff --git a/lib/gitlab/ci/build/response/image.rb b/lib/gitlab/ci/build/response/image.rb deleted file mode 100644 index c160689a2e1..00000000000 --- a/lib/gitlab/ci/build/response/image.rb +++ /dev/null @@ -1,20 +0,0 @@ -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 diff --git a/lib/gitlab/ci/build/response/step.rb b/lib/gitlab/ci/build/response/step.rb deleted file mode 100644 index 98c531c1d99..00000000000 --- a/lib/gitlab/ci/build/response/step.rb +++ /dev/null @@ -1,44 +0,0 @@ -module Gitlab - module Ci - module Build - module Response - class Step - CONDITION_ON_FAILURE = 'on_failure'.freeze - CONDITION_ON_SUCCESS = 'on_success'.freeze - CONDITION_ALWAYS = 'always'.freeze - - attr_reader :name, :script, :when, :allow_failure, :timeout - - class << self - def from_commands(build) - self.new(:script, - build.commands, - build.timeout, - CONDITION_ON_SUCCESS, - false) - 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, - true) - end - end - - def initialize(name, script, timeout, when_condition = CONDITION_ON_SUCCESS, allow_failure = true) - @name = name - @script = script.split("\n") - @timeout = timeout - @when = when_condition - @allow_failure = allow_failure - end - end - end - end - end -end diff --git a/lib/gitlab/ci/build/step.rb b/lib/gitlab/ci/build/step.rb new file mode 100644 index 00000000000..f857ab6063a --- /dev/null +++ b/lib/gitlab/ci/build/step.rb @@ -0,0 +1,56 @@ +module Gitlab + module Ci + module Build + class Step + WHEN_ON_FAILURE = 'on_failure'.freeze + WHEN_ON_SUCCESS = 'on_success'.freeze + WHEN_ALWAYS = 'always'.freeze + + attr_reader :name, :script, :timeout, :when, :allow_failure + + class << self + def from_commands(job) + self.new(:script).tap do |step| + step.script = job.commands + step.timeout = job.timeout + step.when = WHEN_ON_SUCCESS + end + end + + def from_after_script(job) + after_script = job.options[:after_script] + return unless after_script + + self.new(:after_script).tap do |step| + step.script = after_script + step.timeout = job.timeout + step.when = WHEN_ALWAYS + step.allow_failure_on + end + end + end + + def initialize(name) + @name = name + @allow_failure = false + end + + def script=(script) + @script = script.split("\n") + end + + def timeout=(timeout) + @timeout = timeout + end + + def when=(when_condition) + @when = when_condition + end + + def allow_failure_on + @allow_failure = true + end + end + end + end +end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index a77b3356b9a..8ba370c36ea 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -173,5 +173,9 @@ FactoryGirl.define do } end end + + trait :no_options do + options { {} } + end end end diff --git a/spec/lib/gitlab/ci/build/image_spec.rb b/spec/lib/gitlab/ci/build/image_spec.rb new file mode 100644 index 00000000000..c561e4f40e8 --- /dev/null +++ b/spec/lib/gitlab/ci/build/image_spec.rb @@ -0,0 +1,52 @@ +require 'spec_helper' + +describe Gitlab::Ci::Build::Image do + let(:job) { create(:ci_build, :no_options) } + + describe '#from_image' do + subject { described_class.from_image(job) } + + context 'when image is defined in job' do + let(:image_name) { 'ruby:2.1' } + let(:job) { create(:ci_build, options: { image: image_name } ) } + + it { is_expected.to be_kind_of(described_class) } + it { expect(subject.name).to eq(image_name) } + + context 'when image name is empty' do + let(:image_name) { '' } + + it { is_expected.to eq(nil) } + end + end + + context 'when image is not defined in job' do + it { is_expected.to eq(nil) } + end + end + + describe '#from_services' do + subject { described_class.from_services(job) } + + context 'when services are defined in job' do + let(:service_image_name) { 'postgres' } + let(:job) { create(:ci_build, options: { services: [service_image_name] }) } + + it { is_expected.to be_kind_of(Array) } + it { is_expected.not_to be_empty } + it { expect(subject[0].name).to eq(service_image_name) } + + context 'when service image name is empty' do + let(:service_image_name) { '' } + + it { is_expected.to be_kind_of(Array) } + it { is_expected.to be_empty } + end + end + + context 'when services are not defined in job' do + it { is_expected.to be_kind_of(Array) } + it { is_expected.to be_empty } + end + end +end \ No newline at end of file diff --git a/spec/lib/gitlab/ci/build/step_spec.rb b/spec/lib/gitlab/ci/build/step_spec.rb new file mode 100644 index 00000000000..630d7ff6c66 --- /dev/null +++ b/spec/lib/gitlab/ci/build/step_spec.rb @@ -0,0 +1,33 @@ +require 'spec_helper' + +describe Gitlab::Ci::Build::Step do + let(:job) { create(:ci_build, :no_options, commands: "ls -la\ndate") } + + describe '#from_commands' do + subject { described_class.from_commands(job) } + + it { expect(subject.name).to eq(:script) } + it { expect(subject.script).to eq(['ls -la', 'date']) } + it { expect(subject.timeout).to eq(job.timeout) } + it { expect(subject.when).to eq('on_success') } + it { expect(subject.allow_failure).to be_falsey } + end + + describe '#from_after_script' do + subject { described_class.from_after_script(job) } + + context 'when after_script is empty' do + it { is_expected.to be(nil) } + end + + context 'when after_script is not empty' do + let(:job) { create(:ci_build, options: { after_script: "ls -la\ndate" }) } + + it { expect(subject.name).to eq(:after_script) } + it { expect(subject.script).to eq(['ls -la', 'date']) } + it { expect(subject.timeout).to eq(job.timeout) } + it { expect(subject.when).to eq('always') } + it { expect(subject.allow_failure).to be_truthy } + end + end +end \ No newline at end of file diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index f19a9bf354c..f0b6550e48e 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -16,6 +16,7 @@ describe API::Runner do context 'when no token is provided' do it 'returns 400 error' do post api('/runners') + expect(response).to have_http_status 400 end end @@ -23,6 +24,7 @@ describe API::Runner do context 'when invalid token is provided' do it 'returns 403 error' do post api('/runners'), token: 'invalid' + expect(response).to have_http_status 403 end end @@ -108,7 +110,7 @@ describe API::Runner do context "when info parameter '#{param}' info is present" do let(:value) { "#{param}_value" } - it %q(updates provided Runner's parameter) do + it "updates provided Runner's parameter" do post api('/runners'), token: registration_token, info: { param => value } @@ -194,29 +196,34 @@ describe API::Runner do end end - context 'for beta version' do + context 'when beta version is sent' do let(:user_agent) { 'gitlab-runner 9.0.0~beta.167.g2b2bacc (master; go1.7.4; linux/amd64)' } + it { expect(response).to have_http_status(204) } end - context 'for pre-9-0 version' do + context 'when pre-9-0 version is sent' do let(:user_agent) { 'gitlab-ci-multi-runner 1.6.0 (1-6-stable; go1.6.3; linux/amd64)' } + it { expect(response).to have_http_status(204) } end - context 'for pre-9-0 beta version' do + context 'when pre-9-0 beta version is sent' do let(:user_agent) { 'gitlab-ci-multi-runner 1.6.0~beta.167.g2b2bacc (master; go1.6.3; linux/amd64)' } + it { expect(response).to have_http_status(204) } end end - context %q(when runner doesn't send version in User-Agent) do + context "when runner doesn't send version in User-Agent" do let(:user_agent) { 'Go-http-client/1.1' } + it { expect(response).to have_http_status(404) } end - context %q(when runner doesn't have a User-Agent) do + context "when runner doesn't have a User-Agent" do let(:user_agent) { nil } + it { expect(response).to have_http_status(404) } end end @@ -224,6 +231,7 @@ describe API::Runner do context 'when no token is provided' do it 'returns 400 error' do post api('/jobs/request') + expect(response).to have_http_status 400 end end @@ -231,6 +239,7 @@ describe API::Runner do context 'when invalid token is provided' do it 'returns 403 error' do post api('/jobs/request'), token: 'invalid' + expect(response).to have_http_status 403 end end @@ -241,12 +250,14 @@ describe API::Runner do it 'returns 404 error' do request_job + expect(response).to have_http_status 404 end end context 'when jobs are finished' do before { job.success } + it_behaves_like 'no jobs available' end @@ -261,35 +272,64 @@ describe API::Runner do context 'when shared runner requests job for project without shared_runners_enabled' do let(:runner) { create(:ci_runner, :shared) } + it_behaves_like 'no jobs available' end context 'when there is a pending job' do + let(:expected_job_info) do + { 'name' => job.name, + 'stage' => job.stage, + 'project_id' => job.project.id, + 'project_name' => job.project.name } + end + let(:expected_git_info) do + { 'repo_url' => job.repo_url, + 'ref' => job.ref, + 'sha' => job.sha, + 'before_sha' => job.before_sha, + 'ref_type' => 'branch'} + end + let(:expected_steps) do + [{ 'name' => 'script', + 'script' => %w(ls date), + 'timeout' => job.timeout, + 'when' => 'on_success', + 'allow_failure' => false }, + { 'name' => 'after_script', + 'script' => %w(ls date), + 'timeout' => job.timeout, + 'when' => 'always', + 'allow_failure' => true }] + end + let(:expected_variables) do + [{ 'key' => 'CI_BUILD_NAME', 'value' => 'spinach', 'public' => true }, + { 'key' => 'CI_BUILD_STAGE', 'value' => 'test', 'public' => true }, + { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true }] + end + let(:expected_artifacts) do + { 'name' => 'artifacts_file', + 'untracked' => false, + 'paths' => %w(out/), + 'when' => 'always', + 'expire_in' => '7d' } + end + it 'starts a job' do request_job info: { platform: :darwin } expect(response).to have_http_status(201) expect(response.headers).not_to have_key('X-GitLab-Last-Update') 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' => %w(ls date), - 'timeout' => job.timeout, - 'when' => 'always', - 'allow_failure' => true }) - 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/'] }) + expect(json_response['job_info']).to eq(expected_job_info) + expect(json_response['git_info']).to eq(expected_git_info) + expect(json_response['image']).to eq({ 'name' => 'ruby:2.1' }) + expect(json_response['services']).to eq([{ 'name' => 'postgres' }]) + expect(json_response['steps']).to eq(expected_steps) + expect(json_response['artifacts']).to eq(expected_artifacts) + expect(json_response['variables']).to include(*expected_variables) end context 'when job is made for tag' do @@ -297,6 +337,7 @@ describe API::Runner do 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 @@ -305,6 +346,7 @@ describe API::Runner do 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 @@ -318,12 +360,11 @@ describe API::Runner do context "when info parameter '#{param}' is present" do let(:value) { "#{param}_value" } - it %q(updates provided Runner's parameter) do + it "updates provided Runner's parameter" do request_job info: { param => value } expect(response).to have_http_status(201) - runner.reload - expect(runner.read_attribute(param.to_sym)).to eq(value) + expect(runner.reload.read_attribute(param.to_sym)).to eq(value) end end end @@ -336,6 +377,7 @@ describe API::Runner do it 'returns a conflict' do request_job + expect(response).to have_http_status(409) expect(response.headers).not_to have_key('X-GitLab-Last-Update') end @@ -364,17 +406,28 @@ describe API::Runner do it 'picks job' do request_job + expect(response).to have_http_status 201 end end context 'when runner is not allowed to pick untagged jobs' do before { runner.update_column(:run_untagged, false) } + it_behaves_like 'no jobs available' end end context 'when triggered job is available' do + let(:expected_variables) do + [{ 'key' => 'CI_BUILD_NAME', 'value' => 'spinach', 'public' => true }, + { 'key' => 'CI_BUILD_STAGE', 'value' => 'test', 'public' => true }, + { 'key' => 'CI_BUILD_TRIGGERED', 'value' => 'true', 'public' => true }, + { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true }, + { 'key' => 'SECRET_KEY', 'value' => 'secret_value', 'public' => false }, + { 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false }] + end + before do trigger = create(:ci_trigger, project: project) create(:ci_trigger_request_with_variables, pipeline: pipeline, builds: [job], trigger: trigger) @@ -385,12 +438,7 @@ describe API::Runner do request_job expect(response).to have_http_status(201) - expect(json_response['variables']).to include({ 'key' => 'CI_BUILD_NAME', 'value' => 'spinach', 'public' => true }, - { 'key' => 'CI_BUILD_STAGE', 'value' => 'test', 'public' => true }, - { 'key' => 'CI_BUILD_TRIGGERED', 'value' => 'true', 'public' => true }, - { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true }, - { 'key' => 'SECRET_KEY', 'value' => 'secret_value', 'public' => false }, - { 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false }) + expect(json_response['variables']).to include(*expected_variables) end end @@ -419,6 +467,7 @@ describe API::Runner do it 'does not send registry credentials' do request_job + expect(json_response).to have_key('credentials') expect(json_response['credentials']).not_to include(registry_credentials) end @@ -441,11 +490,13 @@ describe API::Runner do context 'when status is given' do it 'mark job as succeeded' do update_job(state: 'success') + expect(job.reload.status).to eq 'success' end it 'mark job as failed' do update_job(state: 'failed') + expect(job.reload.status).to eq 'failed' end end @@ -462,6 +513,7 @@ describe API::Runner do context 'when no trace is given' do it 'does not override trace information' do update_job + expect(job.reload.trace).to eq 'BUILD TRACE' end end @@ -471,6 +523,7 @@ describe API::Runner do it 'responds with forbidden' do update_job + expect(response).to have_http_status(403) end end @@ -502,6 +555,7 @@ describe API::Runner do it "changes the job's trace" do patch_the_trace + expect(job.reload.trace).to eq 'BUILD TRACE appended appended' end @@ -510,6 +564,7 @@ describe API::Runner do it "doesn't change the build.trace" do force_patch_the_trace + expect(job.reload.trace).to eq 'BUILD TRACE appended' end end @@ -522,6 +577,7 @@ describe API::Runner do it 'changes the job.trace' do patch_the_trace + expect(job.reload.trace).to eq 'BUILD TRACE appended appended' end @@ -530,6 +586,7 @@ describe API::Runner do it "doesn't change the job.trace" do force_patch_the_trace + expect(job.reload.trace).to eq 'BUILD TRACE appended' end end @@ -637,6 +694,7 @@ describe API::Runner do it 'fails to post too large artifact' do stub_application_setting(max_artifacts_size: 0) + authorize_artifacts_with_token_in_params(filesize: 100) expect(response).to have_http_status(413) @@ -654,6 +712,7 @@ describe API::Runner do it 'fails to post too large artifact' do stub_application_setting(max_artifacts_size: 0) + authorize_artifacts_with_token_in_headers(filesize: 100) expect(response).to have_http_status(413) @@ -663,19 +722,23 @@ describe API::Runner do context 'when using runners token' do it 'fails to authorize artifacts posting' do authorize_artifacts(token: job.project.runners_token) + expect(response).to have_http_status(403) end end it 'reject requests that did not go through gitlab-workhorse' do headers.delete(Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER) + authorize_artifacts + expect(response).to have_http_status(500) end context 'authorization token is invalid' do it 'responds with forbidden' do authorize_artifacts(token: 'invalid', filesize: 100 ) + expect(response).to have_http_status(403) end end @@ -710,6 +773,7 @@ describe API::Runner do it 'responds with forbidden' do upload_artifacts(file_upload, headers_with_token) + expect(response).to have_http_status(403) end end @@ -745,6 +809,7 @@ describe API::Runner do context 'when using runners token' do it 'responds with forbidden' do upload_artifacts(file_upload, headers.merge(API::Helpers::Runner::JOB_TOKEN_HEADER => job.project.runners_token)) + expect(response).to have_http_status(403) end end @@ -753,7 +818,9 @@ describe API::Runner do context 'when artifacts file is too large' do it 'fails to post too large artifact' do stub_application_setting(max_artifacts_size: 0) + upload_artifacts(file_upload, headers_with_token) + expect(response).to have_http_status(413) end end @@ -761,6 +828,7 @@ describe API::Runner do context 'when artifacts post request does not contain file' do it 'fails to post artifacts without file' do post api("/jobs/#{job.id}/artifacts"), {}, headers_with_token + expect(response).to have_http_status(400) end end @@ -768,6 +836,7 @@ describe API::Runner do context 'GitLab Workhorse is not configured' do it 'fails to post artifacts without GitLab-Workhorse' do post api("/jobs/#{job.id}/artifacts"), { token: job.token }, {} + expect(response).to have_http_status(403) end end @@ -782,6 +851,7 @@ describe API::Runner do before do stub_application_setting(default_artifacts_expire_in: default_artifacts_expire_in) + post(api("/jobs/#{job.id}/artifacts"), post_data, headers_with_token) end @@ -789,9 +859,8 @@ describe API::Runner do let(:expire_in) { '7 days' } it 'updates when specified' do - job.reload expect(response).to have_http_status(201) - expect(job.artifacts_expire_at).to be_within(5.minutes).of(7.days.from_now) + expect(job.reload.artifacts_expire_at).to be_within(5.minutes).of(7.days.from_now) end end @@ -799,9 +868,8 @@ describe API::Runner do let(:expire_in) { nil } it 'ignores if not specified' do - job.reload expect(response).to have_http_status(201) - expect(job.artifacts_expire_at).to be_nil + expect(job.reload.artifacts_expire_at).to be_nil end context 'with application default' do @@ -809,9 +877,8 @@ describe API::Runner do let(:default_artifacts_expire_in) { '5 days' } it 'sets to application default' do - job.reload expect(response).to have_http_status(201) - expect(job.artifacts_expire_at).to be_within(5.minutes).of(5.days.from_now) + expect(job.reload.artifacts_expire_at).to be_within(5.minutes).of(5.days.from_now) end end @@ -819,9 +886,8 @@ describe API::Runner do let(:default_artifacts_expire_in) { '0' } it 'does not set expire_in' do - job.reload expect(response).to have_http_status(201) - expect(job.artifacts_expire_at).to be_nil + expect(job.reload.artifacts_expire_at).to be_nil end end end @@ -884,6 +950,7 @@ describe API::Runner do it' "fails to post artifacts for outside of tmp path"' do upload_artifacts(file_upload, headers_with_token) + expect(response).to have_http_status(400) end end