summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorNick Thomas <nick@gitlab.com>2019-04-09 12:53:07 +0000
committerNick Thomas <nick@gitlab.com>2019-04-09 12:53:07 +0000
commitd95889b8a41037aa68250137b09b22b092776dfe (patch)
tree846932b5b0f86ec5edec5c9ebf2ac5bf8a9731f7
parent41adfde8ffa34eb45bd1d6f15ad6e80c8ef0f8a9 (diff)
parentb5bcf80c9a7470ac36bdbefcb8056beff67712ae (diff)
downloadgitlab-ce-d95889b8a41037aa68250137b09b22b092776dfe.tar.gz
Merge branch '43263-git-push-option-to-create-mr' into 'master'
Git push options to create a merge request, set target_branch and set merge when pipeline succeeds Closes #53198 and #43263 See merge request gitlab-org/gitlab-ce!26752
-rw-r--r--app/services/ci/create_pipeline_service.rb2
-rw-r--r--app/services/git/base_hooks_service.rb2
-rw-r--r--app/services/merge_requests/push_options_handler_service.rb162
-rw-r--r--app/workers/post_receive.rb2
-rw-r--r--changelogs/unreleased/43263-git-push-option-to-create-mr.yml5
-rw-r--r--changelogs/unreleased/53198-git-push-option-merge-when-pipeline-succeeds.yml6
-rw-r--r--doc/user/project/merge_requests/index.md58
-rw-r--r--lib/api/helpers/internal_helpers.rb22
-rw-r--r--lib/api/internal.rb24
-rw-r--r--lib/gitlab/ci/pipeline/chain/skip.rb4
-rw-r--r--lib/gitlab/data_builder/push.rb9
-rw-r--r--lib/gitlab/git_post_receive.rb2
-rw-r--r--lib/gitlab/push_options.rb70
-rw-r--r--spec/lib/gitlab/push_options_spec.rb103
-rw-r--r--spec/requests/api/internal_spec.rb101
-rw-r--r--spec/services/ci/create_pipeline_service_spec.rb3
-rw-r--r--spec/services/merge_requests/push_options_handler_service_spec.rb404
17 files changed, 943 insertions, 36 deletions
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 8973c5ffc9e..41dee4e5641 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -37,7 +37,7 @@ module Ci
variables_attributes: params[:variables_attributes],
project: project,
current_user: current_user,
- push_options: params[:push_options],
+ push_options: params[:push_options] || {},
chat_data: params[:chat_data],
**extra_options(options))
diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb
index e0dccb8716a..fce4040e390 100644
--- a/app/services/git/base_hooks_service.rb
+++ b/app/services/git/base_hooks_service.rb
@@ -79,7 +79,7 @@ module Git
limited_commits,
event_message,
commits_count: commits_count,
- push_options: params[:push_options] || []
+ push_options: params[:push_options] || {}
)
# Dependent code may modify the push data, so return a duplicate each time
diff --git a/app/services/merge_requests/push_options_handler_service.rb b/app/services/merge_requests/push_options_handler_service.rb
new file mode 100644
index 00000000000..d92eb0a68c3
--- /dev/null
+++ b/app/services/merge_requests/push_options_handler_service.rb
@@ -0,0 +1,162 @@
+# frozen_string_literal: true
+
+module MergeRequests
+ class PushOptionsHandlerService
+ LIMIT = 10
+
+ attr_reader :branches, :changes_by_branch, :current_user, :errors,
+ :project, :push_options, :target_project
+
+ def initialize(project, current_user, changes, push_options)
+ @project = project
+ @target_project = @project.default_merge_request_target
+ @current_user = current_user
+ @branches = get_branches(changes)
+ @push_options = push_options
+ @errors = []
+ end
+
+ def execute
+ validate_service
+ return self if errors.present?
+
+ branches.each do |branch|
+ execute_for_branch(branch)
+ rescue Gitlab::Access::AccessDeniedError
+ errors << 'User access was denied'
+ rescue StandardError => e
+ Gitlab::AppLogger.error(e)
+ errors << 'An unknown error occurred'
+ end
+
+ self
+ end
+
+ private
+
+ def get_branches(raw_changes)
+ Gitlab::ChangesList.new(raw_changes).map do |changes|
+ next unless Gitlab::Git.branch_ref?(changes[:ref])
+
+ # Deleted branch
+ next if Gitlab::Git.blank_ref?(changes[:newrev])
+
+ # Default branch
+ branch_name = Gitlab::Git.branch_name(changes[:ref])
+ next if branch_name == target_project.default_branch
+
+ branch_name
+ end.compact.uniq
+ end
+
+ def validate_service
+ errors << 'User is required' if current_user.nil?
+
+ unless target_project.merge_requests_enabled?
+ errors << "Merge requests are not enabled for project #{target_project.full_path}"
+ end
+
+ if branches.size > LIMIT
+ errors << "Too many branches pushed (#{branches.size} were pushed, limit is #{LIMIT})"
+ end
+
+ if push_options[:target] && !target_project.repository.branch_exists?(push_options[:target])
+ errors << "Branch #{push_options[:target]} does not exist"
+ end
+ end
+
+ # Returns a Hash of branch => MergeRequest
+ def merge_requests
+ @merge_requests ||= MergeRequest.from_project(target_project)
+ .opened
+ .from_source_branches(branches)
+ .index_by(&:source_branch)
+ end
+
+ def execute_for_branch(branch)
+ merge_request = merge_requests[branch]
+
+ if merge_request
+ update!(merge_request)
+ else
+ create!(branch)
+ end
+ end
+
+ def create!(branch)
+ unless push_options[:create]
+ errors << "A merge_request.create push option is required to create a merge request for branch #{branch}"
+ return
+ end
+
+ # Use BuildService to assign the standard attributes of a merge request
+ merge_request = ::MergeRequests::BuildService.new(
+ project,
+ current_user,
+ create_params(branch)
+ ).execute
+
+ unless merge_request.errors.present?
+ merge_request = ::MergeRequests::CreateService.new(
+ project,
+ current_user,
+ merge_request.attributes
+ ).execute
+ end
+
+ collect_errors_from_merge_request(merge_request) unless merge_request.persisted?
+ end
+
+ def update!(merge_request)
+ merge_request = ::MergeRequests::UpdateService.new(
+ target_project,
+ current_user,
+ update_params
+ ).execute(merge_request)
+
+ collect_errors_from_merge_request(merge_request) unless merge_request.valid?
+ end
+
+ def create_params(branch)
+ params = {
+ assignee: current_user,
+ source_branch: branch,
+ source_project: project,
+ target_branch: push_options[:target] || target_project.default_branch,
+ target_project: target_project
+ }
+
+ if push_options.key?(:merge_when_pipeline_succeeds)
+ params.merge!(
+ merge_when_pipeline_succeeds: push_options[:merge_when_pipeline_succeeds],
+ merge_user: current_user
+ )
+ end
+
+ params
+ end
+
+ def update_params
+ params = {}
+
+ if push_options.key?(:merge_when_pipeline_succeeds)
+ params.merge!(
+ merge_when_pipeline_succeeds: push_options[:merge_when_pipeline_succeeds],
+ merge_user: current_user
+ )
+ end
+
+ if push_options.key?(:target)
+ params[:target_branch] = push_options[:target]
+ end
+
+ params
+ end
+
+ def collect_errors_from_merge_request(merge_request)
+ merge_request.errors.full_messages.each do |error|
+ errors << error
+ end
+ end
+ end
+end
diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb
index 396f44396a3..a5554f07699 100644
--- a/app/workers/post_receive.rb
+++ b/app/workers/post_receive.rb
@@ -3,7 +3,7 @@
class PostReceive
include ApplicationWorker
- def perform(gl_repository, identifier, changes, push_options = [])
+ def perform(gl_repository, identifier, changes, push_options = {})
project, repo_type = Gitlab::GlRepository.parse(gl_repository)
if project.nil?
diff --git a/changelogs/unreleased/43263-git-push-option-to-create-mr.yml b/changelogs/unreleased/43263-git-push-option-to-create-mr.yml
new file mode 100644
index 00000000000..d50c33da162
--- /dev/null
+++ b/changelogs/unreleased/43263-git-push-option-to-create-mr.yml
@@ -0,0 +1,5 @@
+---
+title: Allow merge requests to be created via git push options
+merge_request: 26752
+author:
+type: added
diff --git a/changelogs/unreleased/53198-git-push-option-merge-when-pipeline-succeeds.yml b/changelogs/unreleased/53198-git-push-option-merge-when-pipeline-succeeds.yml
new file mode 100644
index 00000000000..6fefd05049c
--- /dev/null
+++ b/changelogs/unreleased/53198-git-push-option-merge-when-pipeline-succeeds.yml
@@ -0,0 +1,6 @@
+---
+title: Allow merge requests to be set to merge when pipeline succeeds via git push
+ options
+merge_request: 26842
+author:
+type: added
diff --git a/doc/user/project/merge_requests/index.md b/doc/user/project/merge_requests/index.md
index 7c0380152de..ba7d05a7ad7 100644
--- a/doc/user/project/merge_requests/index.md
+++ b/doc/user/project/merge_requests/index.md
@@ -219,6 +219,64 @@ apply the patches. The target branch can be specified using the
[`/target_branch` quick action](../quick_actions.md). If the source
branch already exists, the patches will be applied on top of it.
+## Git push options
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/26752) in GitLab 11.10.
+
+NOTE: **Note:**
+Git push options are only available with Git 2.10 or newer.
+
+GitLab supports using
+[Git push options](https://git-scm.com/docs/git-push#Documentation/git-push.txt--oltoptiongt)
+to perform the following actions against merge requests at the same time
+as pushing changes:
+
+- Create a new merge request for the pushed branch.
+- Set the target of the merge request to a particular branch.
+- Set the merge request to merge when its pipeline succeeds.
+
+### Create a new merge request using git push options
+
+To create a new merge request for a branch, use the
+`merge_request.create` push option:
+
+```sh
+git push -o merge_request.create
+```
+
+### Set the target branch of a merge request using git push options
+
+To update an existing merge request's target branch, use the
+`merge_request.target=<branch_name>` push option:
+
+```sh
+git push -o merge_request.target=branch_name
+```
+
+You can also create a merge request and set its target branch at the
+same time using a `-o` flag per push option:
+
+```sh
+git push -o merge_request.create -o merge_request.target=branch_name
+```
+
+### Set merge when pipeline succeeds using git push options
+
+To set an existing merge request to
+[merge when its pipeline succeeds](merge_when_pipeline_succeeds.md), use
+the `merge_request.merge_when_pipeline_succeeds` push option:
+
+```sh
+git push -o merge_request.merge_when_pipeline_succeeds
+```
+
+You can also create a merge request and set it to merge when its
+pipeline succeeds at the same time using a `-o` flag per push option:
+
+```sh
+git push -o merge_request.create -o merge_request.merge_when_pipeline_succeeds
+```
+
## Find the merge request that introduced a change
> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/2383) in GitLab 10.5.
diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb
index 3fd824877ae..71c30ec99a5 100644
--- a/lib/api/helpers/internal_helpers.rb
+++ b/lib/api/helpers/internal_helpers.rb
@@ -43,6 +43,28 @@ module API
::MergeRequests::GetUrlsService.new(project).execute(params[:changes])
end
+ def process_mr_push_options(push_options, project, user, changes)
+ output = {}
+
+ service = ::MergeRequests::PushOptionsHandlerService.new(
+ project,
+ user,
+ changes,
+ push_options
+ ).execute
+
+ if service.errors.present?
+ output[:warnings] = push_options_warning(service.errors.join("\n\n"))
+ end
+
+ output
+ end
+
+ def push_options_warning(warning)
+ options = Array.wrap(params[:push_options]).map { |p| "'#{p}'" }.join(' ')
+ "Error encountered with push options #{options}: #{warning}"
+ end
+
def redis_ping
result = Gitlab::Redis::SharedState.with { |redis| redis.ping }
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 9c7b9146c8f..00f0bbab231 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -256,19 +256,27 @@ module API
post '/post_receive' do
status 200
+ output = {} # Messages to gitlab-shell
+ user = identify(params[:identifier])
+ project = Gitlab::GlRepository.parse(params[:gl_repository]).first
+ push_options = Gitlab::PushOptions.new(params[:push_options])
+
PostReceive.perform_async(params[:gl_repository], params[:identifier],
- params[:changes], params[:push_options].to_a)
+ params[:changes], push_options.as_json)
+
+ if Feature.enabled?(:mr_push_options, default_enabled: true)
+ mr_options = push_options.get(:merge_request)
+ output.merge!(process_mr_push_options(mr_options, project, user, params[:changes])) if mr_options.present?
+ end
+
broadcast_message = BroadcastMessage.current&.last&.message
reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease
- output = {
- merge_request_urls: merge_request_urls,
+ output.merge!(
broadcast_message: broadcast_message,
- reference_counter_decreased: reference_counter_decreased
- }
-
- project = Gitlab::GlRepository.parse(params[:gl_repository]).first
- user = identify(params[:identifier])
+ reference_counter_decreased: reference_counter_decreased,
+ merge_request_urls: merge_request_urls
+ )
# A user is not guaranteed to be returned; an orphaned write deploy
# key could be used
diff --git a/lib/gitlab/ci/pipeline/chain/skip.rb b/lib/gitlab/ci/pipeline/chain/skip.rb
index 79bbcc1ed1e..7d6e0704d4a 100644
--- a/lib/gitlab/ci/pipeline/chain/skip.rb
+++ b/lib/gitlab/ci/pipeline/chain/skip.rb
@@ -8,7 +8,6 @@ module Gitlab
include ::Gitlab::Utils::StrongMemoize
SKIP_PATTERN = /\[(ci[ _-]skip|skip[ _-]ci)\]/i
- SKIP_PUSH_OPTION = 'ci.skip'
def perform!
if skipped?
@@ -35,7 +34,8 @@ module Gitlab
end
def push_option_skips_ci?
- !!(@command.push_options&.include?(SKIP_PUSH_OPTION))
+ @command.push_options.present? &&
+ @command.push_options.deep_symbolize_keys.dig(:ci, :skip).present?
end
end
end
diff --git a/lib/gitlab/data_builder/push.rb b/lib/gitlab/data_builder/push.rb
index ea08b5f7eae..af385d7d4ca 100644
--- a/lib/gitlab/data_builder/push.rb
+++ b/lib/gitlab/data_builder/push.rb
@@ -32,10 +32,7 @@ module Gitlab
}
],
total_commits_count: 1,
- push_options: [
- "ci.skip",
- "custom option"
- ]
+ push_options: { ci: { skip: true } }
}.freeze
# Produce a hash of post-receive data
@@ -57,11 +54,11 @@ module Gitlab
# },
# commits: Array,
# total_commits_count: Fixnum,
- # push_options: Array
+ # push_options: Hash
# }
#
# rubocop:disable Metrics/ParameterLists
- def build(project, user, oldrev, newrev, ref, commits = [], message = nil, commits_count: nil, push_options: [])
+ def build(project, user, oldrev, newrev, ref, commits = [], message = nil, commits_count: nil, push_options: {})
commits = Array(commits)
# Total commits count
diff --git a/lib/gitlab/git_post_receive.rb b/lib/gitlab/git_post_receive.rb
index 426436c2164..d98b85fecc4 100644
--- a/lib/gitlab/git_post_receive.rb
+++ b/lib/gitlab/git_post_receive.rb
@@ -5,7 +5,7 @@ module Gitlab
include Gitlab::Identifier
attr_reader :project, :identifier, :changes, :push_options
- def initialize(project, identifier, changes, push_options)
+ def initialize(project, identifier, changes, push_options = {})
@project = project
@identifier = identifier
@changes = deserialize_changes(changes)
diff --git a/lib/gitlab/push_options.rb b/lib/gitlab/push_options.rb
new file mode 100644
index 00000000000..810aba436cc
--- /dev/null
+++ b/lib/gitlab/push_options.rb
@@ -0,0 +1,70 @@
+# frozen_string_literal: true
+
+module Gitlab
+ class PushOptions
+ VALID_OPTIONS = HashWithIndifferentAccess.new({
+ merge_request: {
+ keys: [:create, :merge_when_pipeline_succeeds, :target]
+ },
+ ci: {
+ keys: [:skip]
+ }
+ }).freeze
+
+ NAMESPACE_ALIASES = HashWithIndifferentAccess.new({
+ mr: :merge_request
+ }).freeze
+
+ OPTION_MATCHER = /(?<namespace>[^\.]+)\.(?<key>[^=]+)=?(?<value>.*)/
+
+ attr_reader :options
+
+ def initialize(options = [])
+ @options = parse_options(options)
+ end
+
+ def get(*args)
+ options.dig(*args)
+ end
+
+ # Allow #to_json serialization
+ def as_json(*_args)
+ options
+ end
+
+ private
+
+ def parse_options(raw_options)
+ options = HashWithIndifferentAccess.new
+
+ Array.wrap(raw_options).each do |option|
+ namespace, key, value = parse_option(option)
+
+ next if [namespace, key].any?(&:nil?)
+
+ options[namespace] ||= HashWithIndifferentAccess.new
+ options[namespace][key] = value
+ end
+
+ options
+ end
+
+ def parse_option(option)
+ parts = OPTION_MATCHER.match(option)
+ return unless parts
+
+ namespace, key, value = parts.values_at(:namespace, :key, :value).map(&:strip)
+ namespace = NAMESPACE_ALIASES[namespace] if NAMESPACE_ALIASES[namespace]
+ value = value.presence || true
+
+ return unless valid_option?(namespace, key)
+
+ [namespace, key, value]
+ end
+
+ def valid_option?(namespace, key)
+ keys = VALID_OPTIONS.dig(namespace, :keys)
+ keys && keys.include?(key.to_sym)
+ end
+ end
+end
diff --git a/spec/lib/gitlab/push_options_spec.rb b/spec/lib/gitlab/push_options_spec.rb
new file mode 100644
index 00000000000..fc9e421bea6
--- /dev/null
+++ b/spec/lib/gitlab/push_options_spec.rb
@@ -0,0 +1,103 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::PushOptions do
+ describe 'namespace and key validation' do
+ it 'ignores unrecognised namespaces' do
+ options = described_class.new(['invalid.key=value'])
+
+ expect(options.get(:invalid)).to eq(nil)
+ end
+
+ it 'ignores unrecognised keys' do
+ options = described_class.new(['merge_request.key=value'])
+
+ expect(options.get(:merge_request)).to eq(nil)
+ end
+
+ it 'ignores blank keys' do
+ options = described_class.new(['merge_request'])
+
+ expect(options.get(:merge_request)).to eq(nil)
+ end
+
+ it 'parses recognised namespace and key pairs' do
+ options = described_class.new(['merge_request.target=value'])
+
+ expect(options.get(:merge_request)).to include({
+ target: 'value'
+ })
+ end
+ end
+
+ describe '#get' do
+ it 'can emulate Hash#dig' do
+ options = described_class.new(['merge_request.target=value'])
+
+ expect(options.get(:merge_request, :target)).to eq('value')
+ end
+ end
+
+ describe '#as_json' do
+ it 'returns all options' do
+ options = described_class.new(['merge_request.target=value'])
+
+ expect(options.as_json).to include(
+ merge_request: {
+ target: 'value'
+ }
+ )
+ end
+ end
+
+ it 'can parse multiple push options' do
+ options = described_class.new([
+ 'merge_request.create',
+ 'merge_request.target=value'
+ ])
+
+ expect(options.get(:merge_request)).to include({
+ create: true,
+ target: 'value'
+ })
+ expect(options.get(:merge_request, :create)).to eq(true)
+ expect(options.get(:merge_request, :target)).to eq('value')
+ end
+
+ it 'stores options internally as a HashWithIndifferentAccess' do
+ options = described_class.new([
+ 'merge_request.create'
+ ])
+
+ expect(options.get('merge_request', 'create')).to eq(true)
+ expect(options.get(:merge_request, :create)).to eq(true)
+ end
+
+ it 'selects the last option when options contain duplicate namespace and key pairs' do
+ options = described_class.new([
+ 'merge_request.target=value1',
+ 'merge_request.target=value2'
+ ])
+
+ expect(options.get(:merge_request, :target)).to eq('value2')
+ end
+
+ it 'defaults values to true' do
+ options = described_class.new(['merge_request.create'])
+
+ expect(options.get(:merge_request, :create)).to eq(true)
+ end
+
+ it 'expands aliases' do
+ options = described_class.new(['mr.target=value'])
+
+ expect(options.get(:merge_request, :target)).to eq('value')
+ end
+
+ it 'forgives broken push options' do
+ options = described_class.new(['merge_request . target = value'])
+
+ expect(options.get(:merge_request, :target)).to eq('value')
+ end
+end
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 0919540e4ba..1ce8f520962 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -888,8 +888,10 @@ describe API::Internal do
}
end
+ let(:branch_name) { 'feature' }
+
let(:changes) do
- "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch"
+ "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{branch_name}"
end
let(:push_options) do
@@ -905,9 +907,9 @@ describe API::Internal do
it 'enqueues a PostReceive worker job' do
expect(PostReceive).to receive(:perform_async)
- .with(gl_repository, identifier, changes, push_options)
+ .with(gl_repository, identifier, changes, { ci: { skip: true } })
- post api("/internal/post_receive"), params: valid_params
+ post api('/internal/post_receive'), params: valid_params
end
it 'decreases the reference counter and returns the result' do
@@ -915,17 +917,17 @@ describe API::Internal do
.and_return(reference_counter)
expect(reference_counter).to receive(:decrease).and_return(true)
- post api("/internal/post_receive"), params: valid_params
+ post api('/internal/post_receive'), params: valid_params
expect(json_response['reference_counter_decreased']).to be(true)
end
it 'returns link to create new merge request' do
- post api("/internal/post_receive"), params: valid_params
+ post api('/internal/post_receive'), params: valid_params
expect(json_response['merge_request_urls']).to match [{
- "branch_name" => "new_branch",
- "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
+ "branch_name" => branch_name,
+ "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name}",
"new_merge_request" => true
}]
end
@@ -933,16 +935,87 @@ describe API::Internal do
it 'returns empty array if printing_merge_request_link_enabled is false' do
project.update!(printing_merge_request_link_enabled: false)
- post api("/internal/post_receive"), params: valid_params
+ post api('/internal/post_receive'), params: valid_params
expect(json_response['merge_request_urls']).to eq([])
end
+ it 'does not invoke MergeRequests::PushOptionsHandlerService' do
+ expect(MergeRequests::PushOptionsHandlerService).not_to receive(:new)
+
+ post api('/internal/post_receive'), params: valid_params
+ end
+
+ context 'when there are merge_request push options' do
+ before do
+ valid_params[:push_options] = ['merge_request.create']
+ end
+
+ it 'invokes MergeRequests::PushOptionsHandlerService' do
+ expect(MergeRequests::PushOptionsHandlerService).to receive(:new)
+
+ post api('/internal/post_receive'), params: valid_params
+ end
+
+ it 'creates a new merge request' do
+ expect do
+ post api('/internal/post_receive'), params: valid_params
+ end.to change { MergeRequest.count }.by(1)
+ end
+
+ it 'links to the newly created merge request' do
+ post api('/internal/post_receive'), params: valid_params
+
+ expect(json_response['merge_request_urls']).to match [{
+ 'branch_name' => branch_name,
+ 'url' => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/1",
+ 'new_merge_request' => false
+ }]
+ end
+
+ it 'adds errors on the service instance to warnings' do
+ expect_any_instance_of(
+ MergeRequests::PushOptionsHandlerService
+ ).to receive(:errors).at_least(:once).and_return(['my error'])
+
+ post api('/internal/post_receive'), params: valid_params
+
+ expect(json_response['warnings']).to eq('Error encountered with push options \'merge_request.create\': my error')
+ end
+
+ it 'adds ActiveRecord errors on invalid MergeRequest records to warnings' do
+ invalid_merge_request = MergeRequest.new
+ invalid_merge_request.errors.add(:base, 'my error')
+
+ expect_any_instance_of(
+ MergeRequests::CreateService
+ ).to receive(:execute).and_return(invalid_merge_request)
+
+ post api('/internal/post_receive'), params: valid_params
+
+ expect(json_response['warnings']).to eq('Error encountered with push options \'merge_request.create\': my error')
+ end
+
+ context 'when the feature is disabled' do
+ it 'does not invoke MergeRequests::PushOptionsHandlerService' do
+ Feature.disable(:mr_push_options)
+
+ expect(MergeRequests::PushOptionsHandlerService).to receive(:new)
+
+ expect do
+ post api('/internal/post_receive'), params: valid_params
+ end.not_to change { MergeRequest.count }
+
+ Feature.enable(:mr_push_options)
+ end
+ end
+ end
+
context 'broadcast message exists' do
let!(:broadcast_message) { create(:broadcast_message, starts_at: 1.day.ago, ends_at: 1.day.from_now ) }
it 'returns one broadcast message' do
- post api("/internal/post_receive"), params: valid_params
+ post api('/internal/post_receive'), params: valid_params
expect(response).to have_gitlab_http_status(200)
expect(json_response['broadcast_message']).to eq(broadcast_message.message)
@@ -951,7 +1024,7 @@ describe API::Internal do
context 'broadcast message does not exist' do
it 'returns empty string' do
- post api("/internal/post_receive"), params: valid_params
+ post api('/internal/post_receive'), params: valid_params
expect(response).to have_gitlab_http_status(200)
expect(json_response['broadcast_message']).to eq(nil)
@@ -962,7 +1035,7 @@ describe API::Internal do
it 'returns empty string' do
allow(BroadcastMessage).to receive(:current).and_return(nil)
- post api("/internal/post_receive"), params: valid_params
+ post api('/internal/post_receive'), params: valid_params
expect(response).to have_gitlab_http_status(200)
expect(json_response['broadcast_message']).to eq(nil)
@@ -974,7 +1047,7 @@ describe API::Internal do
project_moved = Gitlab::Checks::ProjectMoved.new(project, user, 'http', 'foo/baz')
project_moved.add_message
- post api("/internal/post_receive"), params: valid_params
+ post api('/internal/post_receive'), params: valid_params
expect(response).to have_gitlab_http_status(200)
expect(json_response["redirected_message"]).to be_present
@@ -987,7 +1060,7 @@ describe API::Internal do
project_created = Gitlab::Checks::ProjectCreated.new(project, user, 'http')
project_created.add_message
- post api("/internal/post_receive"), params: valid_params
+ post api('/internal/post_receive'), params: valid_params
expect(response).to have_gitlab_http_status(200)
expect(json_response["project_created_message"]).to be_present
@@ -999,7 +1072,7 @@ describe API::Internal do
it 'does not try to notify that project moved' do
allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(nil)
- post api("/internal/post_receive"), params: valid_params
+ post api('/internal/post_receive'), params: valid_params
expect(response).to have_gitlab_http_status(200)
end
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index 866d709d446..101b91e9cd8 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -418,8 +418,7 @@ describe Ci::CreatePipelineService do
context 'when push options contain ci.skip' do
let(:push_options) do
- ['ci.skip',
- 'another push option']
+ { 'ci' => { 'skip' => true } }
end
it 'creates a pipline in the skipped state' do
diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb
new file mode 100644
index 00000000000..686b4b49f24
--- /dev/null
+++ b/spec/services/merge_requests/push_options_handler_service_spec.rb
@@ -0,0 +1,404 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::PushOptionsHandlerService do
+ include ProjectForksHelper
+
+ let(:user) { create(:user) }
+ let(:project) { create(:project, :public, :repository) }
+ let(:forked_project) { fork_project(project, user, repository: true) }
+ let(:service) { described_class.new(project, user, changes, push_options) }
+ let(:source_branch) { 'fix' }
+ let(:target_branch) { 'feature' }
+ let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" }
+ let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" }
+ let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" }
+ let(:default_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{project.default_branch}" }
+
+ before do
+ project.add_developer(user)
+ end
+
+ shared_examples_for 'a service that can create a merge request' do
+ subject(:last_mr) { MergeRequest.last }
+
+ it 'creates a merge request' do
+ expect { service.execute }.to change { MergeRequest.count }.by(1)
+ end
+
+ it 'sets the correct target branch' do
+ branch = push_options[:target] || project.default_branch
+
+ service.execute
+
+ expect(last_mr.target_branch).to eq(branch)
+ end
+
+ it 'assigns the MR to the user' do
+ service.execute
+
+ expect(last_mr.assignee).to eq(user)
+ end
+
+ context 'when project has been forked' do
+ let(:forked_project) { fork_project(project, user, repository: true) }
+ let(:service) { described_class.new(forked_project, user, changes, push_options) }
+
+ before do
+ allow(forked_project).to receive(:empty_repo?).and_return(false)
+ end
+
+ it 'sets the correct source project' do
+ service.execute
+
+ expect(last_mr.source_project).to eq(forked_project)
+ end
+
+ it 'sets the correct target project' do
+ service.execute
+
+ expect(last_mr.target_project).to eq(project)
+ end
+ end
+ end
+
+ shared_examples_for 'a service that can set the target of a merge request' do
+ subject(:last_mr) { MergeRequest.last }
+
+ it 'sets the target_branch' do
+ service.execute
+
+ expect(last_mr.target_branch).to eq(target_branch)
+ end
+ end
+
+ shared_examples_for 'a service that can set the merge request to merge when pipeline succeeds' do
+ subject(:last_mr) { MergeRequest.last }
+
+ it 'sets merge_when_pipeline_succeeds' do
+ service.execute
+
+ expect(last_mr.merge_when_pipeline_succeeds).to eq(true)
+ end
+
+ it 'sets merge_user to the user' do
+ service.execute
+
+ expect(last_mr.merge_user).to eq(user)
+ end
+ end
+
+ shared_examples_for 'a service that does not create a merge request' do
+ it do
+ expect { service.execute }.not_to change { MergeRequest.count }
+ end
+ end
+
+ shared_examples_for 'a service that does not update a merge request' do
+ it do
+ expect { service.execute }.not_to change { MergeRequest.maximum(:updated_at) }
+ end
+ end
+
+ shared_examples_for 'a service that does nothing' do
+ include_examples 'a service that does not create a merge request'
+ include_examples 'a service that does not update a merge request'
+ end
+
+ describe '`create` push option' do
+ let(:push_options) { { create: true } }
+
+ context 'with a new branch' do
+ let(:changes) { new_branch_changes }
+
+ it_behaves_like 'a service that can create a merge request'
+ end
+
+ context 'with an existing branch but no open MR' do
+ let(:changes) { existing_branch_changes }
+
+ it_behaves_like 'a service that can create a merge request'
+ end
+
+ context 'with an existing branch that has a merge request open' do
+ let(:changes) { existing_branch_changes }
+ let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)}
+
+ it_behaves_like 'a service that does not create a merge request'
+ end
+
+ context 'with a deleted branch' do
+ let(:changes) { deleted_branch_changes }
+
+ it_behaves_like 'a service that does nothing'
+ end
+
+ context 'with the project default branch' do
+ let(:changes) { default_branch_changes }
+
+ it_behaves_like 'a service that does nothing'
+ end
+ end
+
+ describe '`merge_when_pipeline_succeeds` push option' do
+ let(:push_options) { { merge_when_pipeline_succeeds: true } }
+
+ context 'with a new branch' do
+ let(:changes) { new_branch_changes }
+
+ it_behaves_like 'a service that does not create a merge request'
+
+ it 'adds an error to the service' do
+ error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
+
+ service.execute
+
+ expect(service.errors).to include(error)
+ end
+
+ context 'when coupled with the `create` push option' do
+ let(:push_options) { { create: true, merge_when_pipeline_succeeds: true } }
+
+ it_behaves_like 'a service that can create a merge request'
+ it_behaves_like 'a service that can set the merge request to merge when pipeline succeeds'
+ end
+ end
+
+ context 'with an existing branch but no open MR' do
+ let(:changes) { existing_branch_changes }
+
+ it_behaves_like 'a service that does not create a merge request'
+
+ it 'adds an error to the service' do
+ error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
+
+ service.execute
+
+ expect(service.errors).to include(error)
+ end
+
+ context 'when coupled with the `create` push option' do
+ let(:push_options) { { create: true, merge_when_pipeline_succeeds: true } }
+
+ it_behaves_like 'a service that can create a merge request'
+ it_behaves_like 'a service that can set the merge request to merge when pipeline succeeds'
+ end
+ end
+
+ context 'with an existing branch that has a merge request open' do
+ let(:changes) { existing_branch_changes }
+ let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)}
+
+ it_behaves_like 'a service that does not create a merge request'
+ it_behaves_like 'a service that can set the merge request to merge when pipeline succeeds'
+ end
+
+ context 'with a deleted branch' do
+ let(:changes) { deleted_branch_changes }
+
+ it_behaves_like 'a service that does nothing'
+ end
+
+ context 'with the project default branch' do
+ let(:changes) { default_branch_changes }
+
+ it_behaves_like 'a service that does nothing'
+ end
+ end
+
+ describe '`target` push option' do
+ let(:push_options) { { target: target_branch } }
+
+ context 'with a new branch' do
+ let(:changes) { new_branch_changes }
+
+ it_behaves_like 'a service that does not create a merge request'
+
+ it 'adds an error to the service' do
+ error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
+
+ service.execute
+
+ expect(service.errors).to include(error)
+ end
+
+ context 'when coupled with the `create` push option' do
+ let(:push_options) { { create: true, target: target_branch } }
+
+ it_behaves_like 'a service that can create a merge request'
+ it_behaves_like 'a service that can set the target of a merge request'
+ end
+ end
+
+ context 'with an existing branch but no open MR' do
+ let(:changes) { existing_branch_changes }
+
+ it_behaves_like 'a service that does not create a merge request'
+
+ it 'adds an error to the service' do
+ error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}"
+
+ service.execute
+
+ expect(service.errors).to include(error)
+ end
+
+ context 'when coupled with the `create` push option' do
+ let(:push_options) { { create: true, target: target_branch } }
+
+ it_behaves_like 'a service that can create a merge request'
+ it_behaves_like 'a service that can set the target of a merge request'
+ end
+ end
+
+ context 'with an existing branch that has a merge request open' do
+ let(:changes) { existing_branch_changes }
+ let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)}
+
+ it_behaves_like 'a service that does not create a merge request'
+ it_behaves_like 'a service that can set the target of a merge request'
+ end
+
+ context 'with a deleted branch' do
+ let(:changes) { deleted_branch_changes }
+
+ it_behaves_like 'a service that does nothing'
+ end
+
+ context 'with the project default branch' do
+ let(:changes) { default_branch_changes }
+
+ it_behaves_like 'a service that does nothing'
+ end
+ end
+
+ describe 'multiple pushed branches' do
+ let(:push_options) { { create: true } }
+ let(:changes) do
+ [
+ new_branch_changes,
+ "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/feature_conflict"
+ ]
+ end
+
+ it 'creates a merge request per branch' do
+ expect { service.execute }.to change { MergeRequest.count }.by(2)
+ end
+
+ context 'when there are too many pushed branches' do
+ let(:limit) { MergeRequests::PushOptionsHandlerService::LIMIT }
+ let(:changes) do
+ TestEnv::BRANCH_SHA.to_a[0..limit].map do |x|
+ "#{Gitlab::Git::BLANK_SHA} #{x.first} refs/heads/#{x.last}"
+ end
+ end
+
+ it 'records an error' do
+ service.execute
+
+ expect(service.errors).to eq(["Too many branches pushed (#{limit + 1} were pushed, limit is #{limit})"])
+ end
+ end
+ end
+
+ describe 'no push options' do
+ let(:push_options) { {} }
+ let(:changes) { new_branch_changes }
+
+ it_behaves_like 'a service that does nothing'
+ end
+
+ describe 'no user' do
+ let(:user) { nil }
+ let(:push_options) { { create: true } }
+ let(:changes) { new_branch_changes }
+
+ it 'records an error' do
+ service.execute
+
+ expect(service.errors).to eq(['User is required'])
+ end
+ end
+
+ describe 'unauthorized user' do
+ let(:push_options) { { create: true } }
+ let(:changes) { new_branch_changes }
+
+ it 'records an error' do
+ Members::DestroyService.new(user).execute(ProjectMember.find_by!(user_id: user.id))
+
+ service.execute
+
+ expect(service.errors).to eq(['User access was denied'])
+ end
+ end
+
+ describe 'handling unexpected exceptions' do
+ let(:push_options) { { create: true } }
+ let(:changes) { new_branch_changes }
+ let(:exception) { StandardError.new('My standard error') }
+
+ def run_service_with_exception
+ allow_any_instance_of(
+ MergeRequests::BuildService
+ ).to receive(:execute).and_raise(exception)
+
+ service.execute
+ end
+
+ it 'records an error' do
+ run_service_with_exception
+
+ expect(service.errors).to eq(['An unknown error occurred'])
+ end
+
+ it 'writes to Gitlab::AppLogger' do
+ expect(Gitlab::AppLogger).to receive(:error).with(exception)
+
+ run_service_with_exception
+ end
+ end
+
+ describe 'when target is not a valid branch name' do
+ let(:push_options) { { create: true, target: 'my-branch' } }
+ let(:changes) { new_branch_changes }
+
+ it 'records an error' do
+ service.execute
+
+ expect(service.errors).to eq(['Branch my-branch does not exist'])
+ end
+ end
+
+ describe 'when MRs are not enabled' do
+ let(:push_options) { { create: true } }
+ let(:changes) { new_branch_changes }
+
+ it 'records an error' do
+ expect(project).to receive(:merge_requests_enabled?).and_return(false)
+
+ service.execute
+
+ expect(service.errors).to eq(["Merge requests are not enabled for project #{project.full_path}"])
+ end
+ end
+
+ describe 'when MR has ActiveRecord errors' do
+ let(:push_options) { { create: true } }
+ let(:changes) { new_branch_changes }
+
+ it 'adds the error to its errors property' do
+ invalid_merge_request = MergeRequest.new
+ invalid_merge_request.errors.add(:base, 'my error')
+
+ expect_any_instance_of(
+ MergeRequests::CreateService
+ ).to receive(:execute).and_return(invalid_merge_request)
+
+ service.execute
+
+ expect(service.errors).to eq(['my error'])
+ end
+ end
+end