diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-07-16 16:03:07 +0200 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-07-16 16:03:07 +0200 |
commit | a7fded9b9529cfc08463afd4f6cf12478262e951 (patch) | |
tree | 69a0acd9145acc15a5f90a9bac07f041ccf62e72 | |
parent | 26f5d6047d6e21a5c65a4276266648f1e69aac4a (diff) | |
download | gitlab-ce-a7fded9b9529cfc08463afd4f6cf12478262e951.tar.gz |
Huge refactoring for accepting merge requests
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
20 files changed, 139 insertions, 133 deletions
diff --git a/Gemfile.lock b/Gemfile.lock index 12ba2fa295a..9830f3c77dc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -875,3 +875,6 @@ DEPENDENCIES virtus webmock (~> 1.21.0) wikicloth (= 0.8.1) + +BUNDLED WITH + 1.10.4 diff --git a/app/assets/javascripts/merge_request_widget.js.coffee b/app/assets/javascripts/merge_request_widget.js.coffee index e4d815bb4e4..e0ddc82eb36 100644 --- a/app/assets/javascripts/merge_request_widget.js.coffee +++ b/app/assets/javascripts/merge_request_widget.js.coffee @@ -19,7 +19,7 @@ class @MergeRequestWidget when 'merged' location.reload() else - setTimeout(merge_request_widget.mergeInProgress, 3000) + setTimeout(merge_request_widget.mergeInProgress, 2000) dataType: 'json' getMergeStatus: -> diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 0ef9e99123d..f3054881daf 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -1,7 +1,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController before_action :module_enabled before_action :merge_request, only: [ - :edit, :update, :show, :diffs, :commits, :automerge, :automerge_check, + :edit, :update, :show, :diffs, :commits, :merge, :merge_check, :ci_status, :toggle_subscription ] before_action :closes_issues, only: [:edit, :update, :show, :diffs, :commits] @@ -135,7 +135,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController end end - def automerge_check + def merge_check if @merge_request.unchecked? @merge_request.check_if_can_be_merged end @@ -145,11 +145,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController render partial: "projects/merge_requests/widget/show.html.haml", layout: false end - def automerge + def merge return access_denied! unless @merge_request.can_be_merged_by?(current_user) - if @merge_request.automergeable? - AutoMergeWorker.perform_async(@merge_request.id, current_user.id, params) + if @merge_request.mergeable? + MergeWorker.perform_async(@merge_request.id, current_user.id, params) @status = true else @status = false diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 4dcde029efa..631a2d887cc 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -41,8 +41,6 @@ class MergeRequest < ActiveRecord::Base delegate :commits, :diffs, :last_commit, :last_commit_short_sha, to: :merge_request_diff, prefix: nil - attr_accessor :should_remove_source_branch - # When this attribute is true some MR validation is ignored # It allows us to close or modify broken merge requests attr_accessor :allow_broken @@ -57,7 +55,7 @@ class MergeRequest < ActiveRecord::Base transition [:reopened, :opened] => :closed end - event :merge do + event :mark_as_merged do transition [:reopened, :opened, :locked] => :merged end @@ -223,14 +221,6 @@ class MergeRequest < ActiveRecord::Base self.target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last end - def automerge!(current_user, commit_message = nil) - return unless automergeable? - - MergeRequests::AutoMergeService. - new(target_project, current_user). - execute(self, commit_message) - end - def open? opened? || reopened? end @@ -239,11 +229,11 @@ class MergeRequest < ActiveRecord::Base title =~ /\A\[?WIP\]?:? /i end - def automergeable? + def mergeable? open? && !work_in_progress? && can_be_merged? end - def automerge_status + def gitlab_merge_status if work_in_progress? "work_in_progress" else @@ -445,4 +435,13 @@ class MergeRequest < ActiveRecord::Base "refs/merge-requests/#{id}/head" ) end + + def in_locked_state + begin + lock_mr + yield + ensure + unlock_mr if locked? + end + end end diff --git a/app/models/project_services/gitlab_ci_service.rb b/app/models/project_services/gitlab_ci_service.rb index c284e19fe50..0ea866fdb4a 100644 --- a/app/models/project_services/gitlab_ci_service.rb +++ b/app/models/project_services/gitlab_ci_service.rb @@ -70,6 +70,8 @@ class GitlabCiService < CiService else :error end + rescue Errno::ECONNREFUSED + :error end def fork_registration(new_project, private_token) @@ -99,6 +101,8 @@ class GitlabCiService < CiService if response.code == 200 and response["coverage"] response["coverage"] end + rescue Errno::ECONNREFUSED + nil end def build_page(sha, ref) diff --git a/app/services/base_service.rb b/app/services/base_service.rb index 6d9ed345914..f00ec7408b6 100644 --- a/app/services/base_service.rb +++ b/app/services/base_service.rb @@ -31,6 +31,10 @@ class BaseService SystemHooksService.new end + def repository + project.repository + end + # Add an error to the specified model for restricted visibility levels def deny_visibility_level(model, denied_visibility_level = nil) denied_visibility_level ||= model.visibility_level diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index f587ee266da..d7b40ee8906 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -33,15 +33,8 @@ module Files private - def repository - project.repository - end - def after_commit(sha, branch) - commit = repository.commit(sha) - full_ref = 'refs/heads/' + branch - old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA - GitPushService.new.execute(project, current_user, old_sha, sha, full_ref) + PostCommitService.new(project, current_user).execute(sha, branch) end def current_branch diff --git a/app/services/merge_requests/auto_merge_service.rb b/app/services/merge_requests/auto_merge_service.rb deleted file mode 100644 index 3db498acfcd..00000000000 --- a/app/services/merge_requests/auto_merge_service.rb +++ /dev/null @@ -1,62 +0,0 @@ -module MergeRequests - # AutoMergeService class - # - # Do git merge and in case of success - # mark merge request as merged and execute all hooks and notifications - # Called when you do merge via GitLab UI - class AutoMergeService < BaseMergeService - attr_reader :merge_request, :commit_message - - def execute(merge_request, commit_message) - @commit_message = commit_message - @merge_request = merge_request - - merge_request.lock_mr - - if merge! - merge_request.merge - create_merge_event(merge_request, current_user) - create_note(merge_request) - notification_service.merge_mr(merge_request, current_user) - execute_hooks(merge_request, 'merge') - true - else - merge_request.unlock_mr - false - end - rescue - merge_request.unlock_mr if merge_request.locked? - merge_request.mark_as_unmergeable - false - end - - def merge! - if sha = commit - after_commit(sha, merge_request.target_branch) - end - end - - def commit - committer = repository.user_to_comitter(current_user) - - options = { - message: commit_message, - author: committer, - committer: committer - } - - repository.merge(merge_request.source_sha, merge_request.target_branch, options) - end - - def after_commit(sha, branch) - commit = repository.commit(sha) - full_ref = 'refs/heads/' + branch - old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA - GitPushService.new.execute(project, current_user, old_sha, sha, full_ref) - end - - def repository - project.repository - end - end -end diff --git a/app/services/merge_requests/base_merge_service.rb b/app/services/merge_requests/base_merge_service.rb deleted file mode 100644 index 9579573adf9..00000000000 --- a/app/services/merge_requests/base_merge_service.rb +++ /dev/null @@ -1,10 +0,0 @@ -module MergeRequests - class BaseMergeService < MergeRequests::BaseService - - private - - def create_merge_event(merge_request, current_user) - EventCreateService.new.merge_mr(merge_request, current_user) - end - end -end diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 327ead4ff3f..2107529a21a 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -1,22 +1,57 @@ module MergeRequests # MergeService class # - # Mark existing merge request as merged - # and execute all hooks and notifications - # Called when you do merge via command line and push code - # to target branch - class MergeService < BaseMergeService + # Do git merge and in case of success + # mark merge request as merged and execute all hooks and notifications + # Executed when you do merge via GitLab UI + # + class MergeService < MergeRequests::BaseService + attr_reader :merge_request, :commit_message + def execute(merge_request, commit_message) - merge_request.merge + @commit_message = commit_message + @merge_request = merge_request - create_merge_event(merge_request, current_user) - create_note(merge_request) - notification_service.merge_mr(merge_request, current_user) - execute_hooks(merge_request, 'merge') + unless @merge_request.mergeable? + return error('Merge request is not mergeable') + end + + merge_request.in_locked_state do + if merge_changes + after_merge + success + else + error('Can not merge changes') + end + end + end + + private + + def merge_changes + if sha = commit + after_commit(sha, merge_request.target_branch) + end + end + + def commit + committer = repository.user_to_comitter(current_user) + + options = { + message: commit_message, + author: committer, + committer: committer + } + + repository.merge(merge_request.source_sha, merge_request.target_branch, options) + end + + def after_commit(sha, branch) + PostCommitService.new(project, current_user).execute(sha, branch) + end - true - rescue - false + def after_merge + MergeRequests::PostMergeService.new(project, current_user).execute(merge_request) end end end diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb new file mode 100644 index 00000000000..aceb8cb9021 --- /dev/null +++ b/app/services/merge_requests/post_merge_service.rb @@ -0,0 +1,22 @@ +module MergeRequests + # PostMergeService class + # + # Mark existing merge request as merged + # and execute all hooks and notifications + # + class PostMergeService < MergeRequests::BaseService + def execute(merge_request) + merge_request.mark_as_merged + create_merge_event(merge_request, current_user) + create_note(merge_request) + notification_service.merge_mr(merge_request, current_user) + execute_hooks(merge_request, 'merge') + end + + private + + def create_merge_event(merge_request, current_user) + EventCreateService.new.merge_mr(merge_request, current_user) + end + end +end diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index d0648da049b..8286beaba63 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -33,9 +33,9 @@ module MergeRequests merge_requests.uniq.select(&:source_project).each do |merge_request| - MergeRequests::MergeService. + MergeRequests::PostMergeService new(merge_request.target_project, @current_user). - execute(merge_request, nil) + execute(merge_request) end end diff --git a/app/services/post_commit_service.rb b/app/services/post_commit_service.rb new file mode 100644 index 00000000000..7d7e5fbc32e --- /dev/null +++ b/app/services/post_commit_service.rb @@ -0,0 +1,8 @@ +class PostCommitService < BaseService + def execute(sha, branch) + commit = repository.commit(sha) + full_ref = 'refs/heads/' + branch + old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA + GitPushService.new.execute(project, current_user, old_sha, sha, full_ref) + end +end diff --git a/app/views/projects/merge_requests/automerge.js.haml b/app/views/projects/merge_requests/merge.js.haml index 33321651e32..33321651e32 100644 --- a/app/views/projects/merge_requests/automerge.js.haml +++ b/app/views/projects/merge_requests/merge.js.haml diff --git a/app/views/projects/merge_requests/widget/_show.html.haml b/app/views/projects/merge_requests/widget/_show.html.haml index 263cab7a9e8..a489d4f9b24 100644 --- a/app/views/projects/merge_requests/widget/_show.html.haml +++ b/app/views/projects/merge_requests/widget/_show.html.haml @@ -11,10 +11,10 @@ var merge_request_widget; merge_request_widget = new MergeRequestWidget({ - url_to_automerge_check: "#{automerge_check_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)}", + url_to_automerge_check: "#{merge_check_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)}", check_enable: #{@merge_request.unchecked? ? "true" : "false"}, url_to_ci_check: "#{ci_status_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)}", ci_enable: #{@project.ci_service ? "true" : "false"}, - current_status: "#{@merge_request.automerge_status}", + current_status: "#{@merge_request.gitlab_merge_status}", }); diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml index f5bacaf280a..3c0cd25ba07 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -1,4 +1,4 @@ -= form_for [:automerge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-requires-input' } do |f| += form_for [:merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-requires-input' } do |f| = hidden_field_tag :authenticity_token, form_authenticity_token .accept-merge-holder.clearfix.js-toggle-container .accept-action diff --git a/app/workers/auto_merge_worker.rb b/app/workers/auto_merge_worker.rb deleted file mode 100644 index a6dd73eee5f..00000000000 --- a/app/workers/auto_merge_worker.rb +++ /dev/null @@ -1,13 +0,0 @@ -class AutoMergeWorker - include Sidekiq::Worker - - sidekiq_options queue: :default - - def perform(merge_request_id, current_user_id, params) - params = params.with_indifferent_access - current_user = User.find(current_user_id) - merge_request = MergeRequest.find(merge_request_id) - merge_request.should_remove_source_branch = params[:should_remove_source_branch] - merge_request.automerge!(current_user, params[:commit_message]) - end -end diff --git a/app/workers/merge_worker.rb b/app/workers/merge_worker.rb new file mode 100644 index 00000000000..6a8665c179a --- /dev/null +++ b/app/workers/merge_worker.rb @@ -0,0 +1,19 @@ +class MergeWorker + include Sidekiq::Worker + + sidekiq_options queue: :default + + def perform(merge_request_id, current_user_id, params) + params = params.with_indifferent_access + current_user = User.find(current_user_id) + merge_request = MergeRequest.find(merge_request_id) + + result = MergeRequests::MergeService.new(merge_request.target_project, current_user). + execute(merge_request, params[:commit_message]) + + if result[:status] == :success && params[:should_remove_source_branch].present? + DeleteBranchService.new(merge_request.source_project, current_user). + execute(merge_request.source_branch) + end + end +end diff --git a/config/routes.rb b/config/routes.rb index 055d59a0c93..704cf705870 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -458,8 +458,8 @@ Gitlab::Application.routes.draw do member do get :diffs get :commits - post :automerge - get :automerge_check + post :merge + get :merge_check get :ci_status post :toggle_subscription end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index aa43e1dffd9..a956c960a2e 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -198,7 +198,11 @@ module API if merge_request.open? && !merge_request.work_in_progress? if merge_request.can_be_merged? - merge_request.automerge!(current_user, params[:merge_commit_message] || merge_request.merge_commit_message) + commit_message = params[:merge_commit_message] || merge_request.merge_commit_message + + MergeRequests::MergeService.new(merge_request.target_project, current_user). + execute(merge_request, commit_message) + present merge_request, with: Entities::MergeRequest else render_api_error!('Branch cannot be merged', 405) |