From dd9d8ce82e5957974f87401d6dd437e4d2f3ad38 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 1 Mar 2017 11:39:36 +0100 Subject: Fix runner tags assignment when retrying a job --- app/services/ci/retry_build_service.rb | 31 +++++++++++++++------------- spec/factories/ci/builds.rb | 4 ++++ spec/services/ci/retry_build_service_spec.rb | 29 ++++++++++++++++++-------- 3 files changed, 41 insertions(+), 23 deletions(-) diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb index 38ef323f6e5..001a3e1214a 100644 --- a/app/services/ci/retry_build_service.rb +++ b/app/services/ci/retry_build_service.rb @@ -1,18 +1,21 @@ module Ci class RetryBuildService < ::BaseService - CLONE_ATTRIBUTES = %i[pipeline project ref tag options commands name - allow_failure stage stage_idx trigger_request - yaml_variables when environment coverage_regex] - .freeze - - REJECT_ATTRIBUTES = %i[id status user token coverage trace runner - artifacts_expire_at artifacts_file - artifacts_metadata artifacts_size - created_at updated_at started_at finished_at - queued_at erased_by erased_at].freeze - - IGNORE_ATTRIBUTES = %i[type lock_version gl_project_id target_url - deploy job_id description].freeze + CLONE_ACCESSORS = %i[pipeline project ref tag options commands name + allow_failure stage stage_idx trigger_request + yaml_variables when environment coverage_regex + description tag_list].freeze + + REJECT_ACCESSORS = %i[id status user token coverage trace runner + artifacts_expire_at artifacts_file + artifacts_metadata artifacts_size + created_at updated_at started_at finished_at + queued_at erased_by erased_at].freeze + + IGNORE_ACCESSORS = %i[type lock_version target_url gl_project_id + deploy job_id base_tags commit_id deployments + erased_by_id last_deployment project_id runner_id + tag_taggings taggings tags trigger_request_id + user_id].freeze def execute(build) reprocess(build).tap do |new_build| @@ -31,7 +34,7 @@ module Ci raise Gitlab::Access::AccessDeniedError end - attributes = CLONE_ATTRIBUTES.map do |attribute| + attributes = CLONE_ACCESSORS.map do |attribute| [attribute, build.send(attribute)] end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index a90534d10ba..8218293604d 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -76,6 +76,10 @@ FactoryGirl.define do manual end + trait :tags do + tag_list [:docker, :ruby] + end + after(:build) do |build, evaluator| build.project = build.pipeline.project end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index d03f7505eac..f4bc05377dc 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -13,11 +13,11 @@ describe Ci::RetryBuildService, :services do shared_examples 'build duplication' do let(:build) do create(:ci_build, :failed, :artifacts_expired, :erased, :trace, - :queued, :coverage, pipeline: pipeline) + :queued, :coverage, :tags, pipeline: pipeline) end describe 'clone attributes' do - described_class::CLONE_ATTRIBUTES.each do |attribute| + described_class::CLONE_ACCESSORS.each do |attribute| it "clones #{attribute} build attribute" do expect(new_build.send(attribute)).to eq build.send(attribute) end @@ -25,7 +25,7 @@ describe Ci::RetryBuildService, :services do end describe 'reject attributes' do - described_class::REJECT_ATTRIBUTES.each do |attribute| + described_class::REJECT_ACCESSORS.each do |attribute| it "does not clone #{attribute} build attribute" do expect(new_build.send(attribute)).not_to eq build.send(attribute) end @@ -33,12 +33,23 @@ describe Ci::RetryBuildService, :services do end it 'has correct number of known attributes' do - attributes = - described_class::CLONE_ATTRIBUTES + - described_class::IGNORE_ATTRIBUTES + - described_class::REJECT_ATTRIBUTES - - expect(build.attributes.size).to eq(attributes.size) + known_accessors = + described_class::CLONE_ACCESSORS + + described_class::IGNORE_ACCESSORS + + described_class::REJECT_ACCESSORS + + # :tag_list is a special case, this accessor does not exist + # in reflected associations, comes from `act_as_taggable` and + # we use it to copy tags, instead of reusing tags. + # + current_accessors = + build.attribute_names.map.map(&:to_sym) + + build._reflections.map { |assoc| assoc.first.to_sym } + + [:tag_list] + + current_accessors.uniq! + + expect(known_accessors).to contain_exactly(*current_accessors) end end -- cgit v1.2.1 From 6f84345725ef11493653a888ed59fc6411825779 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 1 Mar 2017 11:57:06 +0100 Subject: Improve build retry service tests for cloning data --- spec/factories/ci/builds.rb | 8 ++++++++ spec/services/ci/retry_build_service_spec.rb | 7 +++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 8218293604d..cabe128acf7 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -80,6 +80,14 @@ FactoryGirl.define do tag_list [:docker, :ruby] end + trait :on_tag do + tag true + end + + trait :triggered do + trigger_request factory: :ci_trigger_request_with_variables + end + after(:build) do |build, evaluator| build.project = build.pipeline.project end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index f4bc05377dc..bcd89544647 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -12,13 +12,16 @@ describe Ci::RetryBuildService, :services do shared_examples 'build duplication' do let(:build) do - create(:ci_build, :failed, :artifacts_expired, :erased, :trace, - :queued, :coverage, :tags, pipeline: pipeline) + create(:ci_build, :failed, :artifacts_expired, :erased, + :queued, :coverage, :tags, :allowed_to_fail, :on_tag, + :teardown_environment, :triggered, :trace, + description: 'some build', pipeline: pipeline) end describe 'clone attributes' do described_class::CLONE_ACCESSORS.each do |attribute| it "clones #{attribute} build attribute" do + expect(new_build.send(attribute)).to be_present expect(new_build.send(attribute)).to eq build.send(attribute) end end -- cgit v1.2.1 From c3d24d0bdd2559715231eca1cd08986277cb8fc6 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 2 Mar 2017 11:51:24 +0100 Subject: Move unused consts from retry build service to specs --- app/services/ci/retry_build_service.rb | 14 +------------ spec/services/ci/retry_build_service_spec.rb | 31 +++++++++++++++++++--------- 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb index 001a3e1214a..ae9ddc35446 100644 --- a/app/services/ci/retry_build_service.rb +++ b/app/services/ci/retry_build_service.rb @@ -3,19 +3,7 @@ module Ci CLONE_ACCESSORS = %i[pipeline project ref tag options commands name allow_failure stage stage_idx trigger_request yaml_variables when environment coverage_regex - description tag_list].freeze - - REJECT_ACCESSORS = %i[id status user token coverage trace runner - artifacts_expire_at artifacts_file - artifacts_metadata artifacts_size - created_at updated_at started_at finished_at - queued_at erased_by erased_at].freeze - - IGNORE_ACCESSORS = %i[type lock_version target_url gl_project_id - deploy job_id base_tags commit_id deployments - erased_by_id last_deployment project_id runner_id - tag_taggings taggings tags trigger_request_id - user_id].freeze + description tag_list].freeze def execute(build) reprocess(build).tap do |new_build| diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index bcd89544647..65af4e13118 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -10,6 +10,20 @@ describe Ci::RetryBuildService, :services do described_class.new(project, user) end + CLONE_ACCESSORS = described_class::CLONE_ACCESSORS + + REJECT_ACCESSORS = + %i[id status user token coverage trace runner artifacts_expire_at + artifacts_file artifacts_metadata artifacts_size created_at + updated_at started_at finished_at queued_at erased_by + erased_at].freeze + + IGNORE_ACCESSORS = + %i[type lock_version target_url gl_project_id deploy job_id base_tags + commit_id deployments erased_by_id last_deployment project_id + runner_id tag_taggings taggings tags trigger_request_id + user_id].freeze + shared_examples 'build duplication' do let(:build) do create(:ci_build, :failed, :artifacts_expired, :erased, @@ -18,8 +32,8 @@ describe Ci::RetryBuildService, :services do description: 'some build', pipeline: pipeline) end - describe 'clone attributes' do - described_class::CLONE_ACCESSORS.each do |attribute| + describe 'clone accessors' do + CLONE_ACCESSORS.each do |attribute| it "clones #{attribute} build attribute" do expect(new_build.send(attribute)).to be_present expect(new_build.send(attribute)).to eq build.send(attribute) @@ -27,8 +41,8 @@ describe Ci::RetryBuildService, :services do end end - describe 'reject attributes' do - described_class::REJECT_ACCESSORS.each do |attribute| + describe 'reject acessors' do + REJECT_ACCESSORS.each do |attribute| it "does not clone #{attribute} build attribute" do expect(new_build.send(attribute)).not_to eq build.send(attribute) end @@ -36,18 +50,15 @@ describe Ci::RetryBuildService, :services do end it 'has correct number of known attributes' do - known_accessors = - described_class::CLONE_ACCESSORS + - described_class::IGNORE_ACCESSORS + - described_class::REJECT_ACCESSORS + known_accessors = CLONE_ACCESSORS + REJECT_ACCESSORS + IGNORE_ACCESSORS # :tag_list is a special case, this accessor does not exist # in reflected associations, comes from `act_as_taggable` and # we use it to copy tags, instead of reusing tags. # current_accessors = - build.attribute_names.map.map(&:to_sym) + - build._reflections.map { |assoc| assoc.first.to_sym } + + Ci::Build.attribute_names.map(&:to_sym) + + Ci::Build.reflect_on_all_associations.map(&:name) + [:tag_list] current_accessors.uniq! -- cgit v1.2.1 From f0ba1ea3c1157d9aae472872481982ac53fb90a8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 2 Mar 2017 22:14:12 +0100 Subject: Fix Rubocop offense in build retry service --- app/services/ci/retry_build_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb index ae9ddc35446..89da05b72bb 100644 --- a/app/services/ci/retry_build_service.rb +++ b/app/services/ci/retry_build_service.rb @@ -3,7 +3,7 @@ module Ci CLONE_ACCESSORS = %i[pipeline project ref tag options commands name allow_failure stage stage_idx trigger_request yaml_variables when environment coverage_regex - description tag_list].freeze + description tag_list].freeze def execute(build) reprocess(build).tap do |new_build| -- cgit v1.2.1