From a39277a4f9cd11029601bf863e09a127f8e82291 Mon Sep 17 00:00:00 2001 From: Raphael Tweitmann Date: Mon, 17 Jun 2019 12:00:51 +0000 Subject: [PATCH] Extract common validations from ci services DroneCI and TeamCity shared the same validations methods on the data received. These validations were extracted into a concern --- .../concerns/service_push_data_validations.rb | 43 +++++++++++++++++++ .../project_services/drone_ci_service.rb | 20 +-------- .../project_services/teamcity_service.rb | 42 ++++++------------ .../project_services/teamcity_service_spec.rb | 15 +++++-- 4 files changed, 68 insertions(+), 52 deletions(-) create mode 100644 app/models/concerns/service_push_data_validations.rb diff --git a/app/models/concerns/service_push_data_validations.rb b/app/models/concerns/service_push_data_validations.rb new file mode 100644 index 00000000000..defc5794142 --- /dev/null +++ b/app/models/concerns/service_push_data_validations.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +# This concern is used by registerd services such as TeamCityService and +# DroneCiService and add methods to perform validations on the received +# data. + +module ServicePushDataValidations + extend ActiveSupport::Concern + + def merge_request_valid?(data) + data.dig(:object_attributes, :state) == 'opened' && merge_request_unchecked?(data) + end + + def push_valid?(data) + data[:total_commits_count] > 0 && + !branch_removed?(data) && + # prefer merge request trigger over push to avoid double builds + !opened_merge_requests?(data) + end + + def tag_push_valid?(data) + data[:total_commits_count] > 0 && !branch_removed?(data) + end + + private + + def branch_removed?(data) + Gitlab::Git.blank_ref?(data[:after]) + end + + def opened_merge_requests?(data) + project.merge_requests + .opened + .from_project(project) + .from_source_branches(Gitlab::Git.ref_name(data[:ref])) + .exists? + end + + def merge_request_unchecked?(data) + MergeRequest.state_machines[:merge_status] + .check_state?(data.dig(:object_attributes, :merge_status)) + end +end diff --git a/app/models/project_services/drone_ci_service.rb b/app/models/project_services/drone_ci_service.rb index 5ccc2f019cb..dbdc8345c93 100644 --- a/app/models/project_services/drone_ci_service.rb +++ b/app/models/project_services/drone_ci_service.rb @@ -2,6 +2,7 @@ class DroneCiService < CiService include ReactiveService + include ServicePushDataValidations prop_accessor :drone_url, :token boolean_accessor :enable_ssl_verification @@ -96,23 +97,4 @@ class DroneCiService < CiService { type: 'checkbox', name: 'enable_ssl_verification', title: "Enable SSL verification" } ] end - - private - - def tag_push_valid?(data) - data[:total_commits_count] > 0 && !Gitlab::Git.blank_ref?(data[:after]) - end - - def push_valid?(data) - opened_merge_requests = project.merge_requests.opened.where(source_project_id: project.id, - source_branch: Gitlab::Git.ref_name(data[:ref])) - - opened_merge_requests.empty? && data[:total_commits_count] > 0 && - !Gitlab::Git.blank_ref?(data[:after]) - end - - def merge_request_valid?(data) - data[:object_attributes][:state] == 'opened' && - MergeRequest.state_machines[:merge_status].check_state?(data[:object_attributes][:merge_status]) - end end diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb index 918a1b32612..68c07fa37f2 100644 --- a/app/models/project_services/teamcity_service.rb +++ b/app/models/project_services/teamcity_service.rb @@ -2,6 +2,7 @@ class TeamcityService < CiService include ReactiveService + include ServicePushDataValidations prop_accessor :teamcity_url, :build_type, :username, :password @@ -89,20 +90,26 @@ class TeamcityService < CiService end def execute(data) - return unless supported_events.include?(data[:object_kind]) - case data[:object_kind] when 'push' - branch = Gitlab::Git.ref_name(data[:ref]) - post_to_build_queue(data, branch) if push_valid?(data) + execute_push(data) when 'merge_request' - branch = data[:object_attributes][:source_branch] - post_to_build_queue(data, branch) if merge_request_valid?(data) + execute_merge_request(data) end end private + def execute_push(data) + branch = Gitlab::Git.ref_name(data[:ref]) + post_to_build_queue(data, branch) if push_valid?(data) + end + + def execute_merge_request(data) + branch = data[:object_attributes][:source_branch] + post_to_build_queue(data, branch) if merge_request_valid?(data) + end + def read_build_page(response) if response.code != 200 # If actual build link can't be determined, @@ -159,27 +166,4 @@ class TeamcityService < CiService def basic_auth { username: username, password: password } end - - def push_valid?(data) - data[:total_commits_count] > 0 && - !branch_removed?(data) && - no_open_merge_requests?(data) - end - - def merge_request_valid?(data) - data.dig(:object_attributes, :state) == 'opened' && - MergeRequest.state_machines[:merge_status].check_state?(data.dig(:object_attributes, :merge_status)) - end - - def branch_removed?(data) - Gitlab::Git.blank_ref?(data[:after]) - end - - def no_open_merge_requests?(data) - !project.merge_requests - .opened - .from_project(project) - .from_source_branches(Gitlab::Git.ref_name(data[:ref])) - .exists? - end end diff --git a/spec/models/project_services/teamcity_service_spec.rb b/spec/models/project_services/teamcity_service_spec.rb index 1edb17932e5..3d875bc49e7 100644 --- a/spec/models/project_services/teamcity_service_spec.rb +++ b/spec/models/project_services/teamcity_service_spec.rb @@ -7,10 +7,11 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do include StubRequests let(:teamcity_url) { 'http://gitlab.com/teamcity' } + let(:project) { create(:project) } subject(:service) do described_class.create( - project: create(:project), + project: project, properties: { teamcity_url: teamcity_url, username: 'mic', @@ -225,7 +226,7 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do end it 'returns nil when ref is blank' do - data[:after] = "0000000000000000000000000000000000000000" + data[:after] = Gitlab::Git::BLANK_SHA expect(service.execute(data)).to be_nil end @@ -235,6 +236,12 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do expect(service.execute(data)).to be_nil end + + it 'returns nil when a merge request is opened for the same ref' do + create(:merge_request, source_project: project, source_branch: 'dev-123_branch') + + expect(service.execute(data)).to be_nil + end end context 'when merge_request' do @@ -264,8 +271,8 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do expect(service.execute(data)).to be_nil end - it 'returns nil when merge request is not unchecked or cannot_be_merged_recheck' do - data[:object_attributes][:merge_status] = 'checked' + it 'returns nil unless merge request is marked as unchecked' do + data[:object_attributes][:merge_status] = 'can_be_merged' expect(service.execute(data)).to be_nil end