From 1fcf3f3d94a157e2b57735ca9d0a306b91e75323 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 2 Aug 2017 22:11:32 +0900 Subject: [PATCH 1/5] essential --- app/services/merge_requests/create_service.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 19189e64acf..5414fa79def 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -12,7 +12,6 @@ module MergeRequests merge_request.source_project = source_project merge_request.source_branch = params[:source_branch] merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) - merge_request.head_pipeline = head_pipeline_for(merge_request) create(merge_request) end @@ -22,10 +21,16 @@ module MergeRequests notification_service.new_merge_request(issuable, current_user) todo_service.new_merge_request(issuable, current_user) issuable.cache_merge_request_closes_issues!(current_user) + update_merge_requests_head_pipeline(issuable) end private + def update_merge_requests_head_pipeline(merge_request) + pipeline = head_pipeline_for(merge_request) + merge_request.update(head_pipeline_id: pipeline.id) if pipeline + end + def head_pipeline_for(merge_request) return unless merge_request.source_project From 28b442989b7d03432d300bedac89e5971d9a69b7 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 4 Aug 2017 14:15:38 +0900 Subject: [PATCH 2/5] Add changelog --- ...x-sm-34547-cannot-connect-to-ci-server-error-messages.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/fix-sm-34547-cannot-connect-to-ci-server-error-messages.yml diff --git a/changelogs/unreleased/fix-sm-34547-cannot-connect-to-ci-server-error-messages.yml b/changelogs/unreleased/fix-sm-34547-cannot-connect-to-ci-server-error-messages.yml new file mode 100644 index 00000000000..ddaec4f19f9 --- /dev/null +++ b/changelogs/unreleased/fix-sm-34547-cannot-connect-to-ci-server-error-messages.yml @@ -0,0 +1,5 @@ +--- +title: Fix an order of operations for CI connection error message in merge request + widget +merge_request: 13252 +author: From f2f1b9e3dd10f1f527660e131fb307c68214246a Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 4 Aug 2017 18:01:59 +0900 Subject: [PATCH 3/5] Add a spec for concurrent process --- .../features/merge_requests/pipelines_spec.rb | 97 +++++++++++++------ 1 file changed, 69 insertions(+), 28 deletions(-) diff --git a/spec/features/merge_requests/pipelines_spec.rb b/spec/features/merge_requests/pipelines_spec.rb index b3d6cf8deb4..2136f7d829c 100644 --- a/spec/features/merge_requests/pipelines_spec.rb +++ b/spec/features/merge_requests/pipelines_spec.rb @@ -1,45 +1,86 @@ require 'spec_helper' feature 'Pipelines for Merge Requests', js: true do - given(:user) { create(:user) } - given(:merge_request) { create(:merge_request) } - given(:project) { merge_request.target_project } - - before do - project.team << [user, :master] - sign_in user - end - - context 'with pipelines' do - let!(:pipeline) do - create(:ci_empty_pipeline, - project: merge_request.source_project, - ref: merge_request.source_branch, - sha: merge_request.diff_head_sha) - end + describe 'pipeline tab' do + given(:user) { create(:user) } + given(:merge_request) { create(:merge_request) } + given(:project) { merge_request.target_project } before do - visit project_merge_request_path(project, merge_request) + project.team << [user, :master] + sign_in user end - scenario 'user visits merge request pipelines tab' do - page.within('.merge-request-tabs') do - click_link('Pipelines') + context 'with pipelines' do + let!(:pipeline) do + create(:ci_empty_pipeline, + project: merge_request.source_project, + ref: merge_request.source_branch, + sha: merge_request.diff_head_sha) end - wait_for_requests - expect(page).to have_selector('.stage-cell') + before do + visit project_merge_request_path(project, merge_request) + end + + scenario 'user visits merge request pipelines tab' do + page.within('.merge-request-tabs') do + click_link('Pipelines') + end + wait_for_requests + + expect(page).to have_selector('.sssssssss') + end + end + + context 'without pipelines' do + before do + visit project_merge_request_path(project, merge_request) + end + + scenario 'user visits merge request page' do + page.within('.merge-request-tabs') do + expect(page).to have_no_link('Pipelines') + end + end end end - context 'without pipelines' do - before do - visit project_merge_request_path(project, merge_request) + describe 'race condition' do + given(:project) { create(:project, :repository) } + given(:user) { create(:user) } + given(:build_push_data) { { ref: 'feature', checkout_sha: TestEnv::BRANCH_SHA['feature'] } } + + given(:merge_request_params) do + { "source_branch" => "feature", "source_project_id" => project.id, + "target_branch" => "master", "target_project_id" => project.id, "title" => "A" } end - scenario 'user visits merge request page' do - page.within('.merge-request-tabs') do - expect(page).to have_no_link('Pipelines') + background do + project.add_master(user) + sign_in user + end + + context 'when pipeline and merge request were created simultaneously' do + background do + stub_ci_pipeline_to_return_yaml_file + + threads = [] + threads << Thread.new do + @merge_request = MergeRequests::CreateService.new(project, user, merge_request_params).execute + end + + threads << Thread.new do + @pipeline = Ci::CreatePipelineService.new(project, user, build_push_data).execute(:push) + end + + threads.each { |thr| thr.join } + end + + scenario 'user sees pipeline in merge request widget' do + visit project_merge_request_path(project, @merge_request) + expect(page.find(".ci-widget")).to have_content(TestEnv::BRANCH_SHA['feature']) + expect(page.find(".ci-widget")).to have_content("##{@pipeline.id}") end end end From 1b4fd63cf26ae0f19ec7735fd56bc0df6ce07058 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 4 Aug 2017 20:18:05 +0900 Subject: [PATCH 4/5] Fix spec --- spec/features/merge_requests/pipelines_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/features/merge_requests/pipelines_spec.rb b/spec/features/merge_requests/pipelines_spec.rb index 2136f7d829c..44b965d5e6a 100644 --- a/spec/features/merge_requests/pipelines_spec.rb +++ b/spec/features/merge_requests/pipelines_spec.rb @@ -29,7 +29,7 @@ feature 'Pipelines for Merge Requests', js: true do end wait_for_requests - expect(page).to have_selector('.sssssssss') + expect(page).to have_selector('.stage-cell') end end @@ -66,6 +66,7 @@ feature 'Pipelines for Merge Requests', js: true do stub_ci_pipeline_to_return_yaml_file threads = [] + threads << Thread.new do @merge_request = MergeRequests::CreateService.new(project, user, merge_request_params).execute end @@ -79,6 +80,7 @@ feature 'Pipelines for Merge Requests', js: true do scenario 'user sees pipeline in merge request widget' do visit project_merge_request_path(project, @merge_request) + expect(page.find(".ci-widget")).to have_content(TestEnv::BRANCH_SHA['feature']) expect(page.find(".ci-widget")).to have_content("##{@pipeline.id}") end From 1614eb8b0a9fb2d719e6af7fec13d76db7d99892 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 4 Aug 2017 21:44:19 +0900 Subject: [PATCH 5/5] Fix spec --- spec/features/merge_requests/pipelines_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/features/merge_requests/pipelines_spec.rb b/spec/features/merge_requests/pipelines_spec.rb index 44b965d5e6a..347ce788b36 100644 --- a/spec/features/merge_requests/pipelines_spec.rb +++ b/spec/features/merge_requests/pipelines_spec.rb @@ -52,8 +52,8 @@ feature 'Pipelines for Merge Requests', js: true do given(:build_push_data) { { ref: 'feature', checkout_sha: TestEnv::BRANCH_SHA['feature'] } } given(:merge_request_params) do - { "source_branch" => "feature", "source_project_id" => project.id, - "target_branch" => "master", "target_project_id" => project.id, "title" => "A" } + { "source_branch" => "feature", "source_project_id" => project.id, + "target_branch" => "master", "target_project_id" => project.id, "title" => "A" } end background do