diff options
Diffstat (limited to 'app/services/merge_requests')
12 files changed, 224 insertions, 113 deletions
diff --git a/app/services/merge_requests/after_create_service.rb b/app/services/merge_requests/after_create_service.rb index 93a0d375b97..9d12eb80eb6 100644 --- a/app/services/merge_requests/after_create_service.rb +++ b/app/services/merge_requests/after_create_service.rb @@ -28,7 +28,7 @@ module MergeRequests merge_request.diffs(include_stats: false).write_cache merge_request.create_cross_references!(current_user) - OnboardingProgressService.new(merge_request.target_project.namespace).execute(action: :merge_request_created) + Onboarding::ProgressService.new(merge_request.target_project.namespace).execute(action: :merge_request_created) todo_service.new_merge_request(merge_request, current_user) merge_request.cache_merge_request_closes_issues!(current_user) diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb index dcc4cf4bb1e..64ae33c9b15 100644 --- a/app/services/merge_requests/approval_service.rb +++ b/app/services/merge_requests/approval_service.rb @@ -17,19 +17,11 @@ module MergeRequests # utilizing the `Gitlab::EventStore`. # # Workers can subscribe to the `MergeRequests::ApprovedEvent`. - if Feature.enabled?(:async_after_approval, project) - Gitlab::EventStore.publish( - MergeRequests::ApprovedEvent.new( - data: { current_user_id: current_user.id, merge_request_id: merge_request.id } - ) + Gitlab::EventStore.publish( + MergeRequests::ApprovedEvent.new( + data: { current_user_id: current_user.id, merge_request_id: merge_request.id } ) - else - create_event(merge_request) - stream_audit_event(merge_request) - create_approval_note(merge_request) - mark_pending_todos_as_done(merge_request) - execute_approval_hooks(merge_request, current_user) - end + ) success end @@ -37,7 +29,7 @@ module MergeRequests private def can_be_approved?(merge_request) - current_user.can?(:approve_merge_request, merge_request) + merge_request.can_be_approved_by?(current_user) end def save_approval(approval) @@ -49,29 +41,6 @@ module MergeRequests def reset_approvals_cache(merge_request) merge_request.approvals.reset end - - def create_event(merge_request) - event_service.approve_mr(merge_request, current_user) - end - - def stream_audit_event(merge_request) - # Defined in EE - end - - def create_approval_note(merge_request) - SystemNoteService.approve_mr(merge_request, current_user) - end - - def mark_pending_todos_as_done(merge_request) - todo_service.resolve_todos_for_target(merge_request, current_user) - end - - def execute_approval_hooks(merge_request, current_user) - # Only one approval is required for a merge request to be approved - notification_service.async.approve_mr(merge_request, current_user) - - execute_hooks(merge_request, 'approved') - end end end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index bda8dc64ac0..6cefd9169f5 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -43,8 +43,6 @@ module MergeRequests end def handle_assignees_change(merge_request, old_assignees) - bulk_update_assignees_state(merge_request, merge_request.assignees - old_assignees) - MergeRequests::HandleAssigneesChangeService .new(project: project, current_user: current_user) .async_execute(merge_request, old_assignees) @@ -60,7 +58,6 @@ module MergeRequests new_reviewers = merge_request.reviewers - old_reviewers merge_request_activity_counter.track_users_review_requested(users: new_reviewers) merge_request_activity_counter.track_reviewers_changed_action(user: current_user) - bulk_update_reviewers_state(merge_request, new_reviewers) end def cleanup_environments(merge_request) @@ -247,46 +244,6 @@ module MergeRequests Milestones::MergeRequestsCountService.new(milestone).delete_cache end - - def bulk_update_assignees_state(merge_request, new_assignees) - return unless current_user.mr_attention_requests_enabled? - return if new_assignees.empty? - - assignees_map = merge_request.merge_request_assignees_with(new_assignees).to_h do |assignee| - state = if assignee.user_id == current_user&.id - :unreviewed - else - merge_request.find_reviewer(assignee.assignee)&.state || :attention_requested - end - - [ - assignee, - { state: MergeRequestAssignee.states[state], updated_state_by_user_id: current_user.id } - ] - end - - ::Gitlab::Database::BulkUpdate.execute(%i[state updated_state_by_user_id], assignees_map) - end - - def bulk_update_reviewers_state(merge_request, new_reviewers) - return unless current_user.mr_attention_requests_enabled? - return if new_reviewers.empty? - - reviewers_map = merge_request.merge_request_reviewers_with(new_reviewers).to_h do |reviewer| - state = if reviewer.user_id == current_user&.id - :unreviewed - else - merge_request.find_assignee(reviewer.reviewer)&.state || :attention_requested - end - - [ - reviewer, - { state: MergeRequestReviewer.states[state], updated_state_by_user_id: current_user.id } - ] - end - - ::Gitlab::Database::BulkUpdate.execute(%i[state updated_state_by_user_id], reviewers_map) - end end end diff --git a/app/services/merge_requests/ff_merge_service.rb b/app/services/merge_requests/ff_merge_service.rb index c5640047899..6e1d1b6ad23 100644 --- a/app/services/merge_requests/ff_merge_service.rb +++ b/app/services/merge_requests/ff_merge_service.rb @@ -8,26 +8,22 @@ module MergeRequests # Executed when you do fast-forward merge via GitLab UI # class FfMergeService < MergeRequests::MergeService - private + extend ::Gitlab::Utils::Override - def commit - ff_merge = repository.ff_merge(current_user, - source, - merge_request.target_branch, - merge_request: merge_request) + private - if merge_request.squash_on_merge? - merge_request.update_column(:squash_commit_sha, merge_request.in_progress_merge_commit_sha) - end + override :execute_git_merge + def execute_git_merge + repository.ff_merge(current_user, + source, + merge_request.target_branch, + merge_request: merge_request) + end - ff_merge - rescue Gitlab::Git::PreReceiveError => e - Gitlab::ErrorTracking.track_exception(e, pre_receive_message: e.raw_message, merge_request_id: merge_request&.id) - raise MergeError, e.message - rescue StandardError => e - raise MergeError, "Something went wrong during merge: #{e.message}" - ensure - merge_request.update_and_mark_in_progress_merge_commit_sha(nil) + override :merge_success_data + def merge_success_data(commit_id) + # There is no merge commit to update, so this is just blank. + {} end end end diff --git a/app/services/merge_requests/handle_assignees_change_service.rb b/app/services/merge_requests/handle_assignees_change_service.rb index 87cd6544406..51be4690af4 100644 --- a/app/services/merge_requests/handle_assignees_change_service.rb +++ b/app/services/merge_requests/handle_assignees_change_service.rb @@ -21,7 +21,7 @@ module MergeRequests merge_request_activity_counter.track_users_assigned_to_mr(users: new_assignees) merge_request_activity_counter.track_assignees_changed_action(user: current_user) - execute_assignees_hooks(merge_request, old_assignees) if options[:execute_hooks] + execute_assignees_hooks(merge_request, old_assignees) if options['execute_hooks'] end private diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index f51923b7035..6d31a29f5a7 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -92,15 +92,26 @@ module MergeRequests raise_error(GENERIC_ERROR_MESSAGE) end - merge_request.update!(merge_commit_sha: commit_id) + update_merge_sha_metadata(commit_id) + + commit_id ensure merge_request.update_and_mark_in_progress_merge_commit_sha(nil) end + def update_merge_sha_metadata(commit_id) + data_to_update = merge_success_data(commit_id) + data_to_update[:squash_commit_sha] = source if merge_request.squash_on_merge? + + merge_request.update!(**data_to_update) if data_to_update.present? + end + + def merge_success_data(commit_id) + { merge_commit_sha: commit_id } + end + def try_merge - repository.merge(current_user, source, merge_request, commit_message).tap do - merge_request.update_column(:squash_commit_sha, source) if merge_request.squash_on_merge? - end + execute_git_merge rescue Gitlab::Git::PreReceiveError => e raise MergeError, "Something went wrong during merge pre-receive hook. #{e.message}".strip @@ -109,6 +120,10 @@ module MergeRequests raise_error(GENERIC_ERROR_MESSAGE) end + def execute_git_merge + repository.merge(current_user, source, merge_request, commit_message) + end + def after_merge log_info("Post merge started on JID #{merge_jid} with state #{state}") MergeRequests::PostMergeService.new(project: project, current_user: current_user).execute(merge_request) diff --git a/app/services/merge_requests/mergeability/detailed_merge_status_service.rb b/app/services/merge_requests/mergeability/detailed_merge_status_service.rb new file mode 100644 index 00000000000..d25234183fd --- /dev/null +++ b/app/services/merge_requests/mergeability/detailed_merge_status_service.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +module MergeRequests + module Mergeability + class DetailedMergeStatusService + include ::Gitlab::Utils::StrongMemoize + + def initialize(merge_request:) + @merge_request = merge_request + end + + def execute + return :checking if checking? + return :unchecked if unchecked? + + if check_results.success? + + # If everything else is mergeable, but CI is not, the frontend expects two potential states to be returned + # See discussion: gitlab.com/gitlab-org/gitlab/-/merge_requests/96778#note_1093063523 + if check_ci_results.success? + :mergeable + else + ci_check_failure_reason + end + else + check_results.failure_reason + end + end + + private + + attr_reader :merge_request, :checks, :ci_check + + def checking? + merge_request.cannot_be_merged_rechecking? || merge_request.preparing? || merge_request.checking? + end + + def unchecked? + merge_request.unchecked? + end + + def check_results + strong_memoize(:check_results) do + merge_request.execute_merge_checks(params: { skip_ci_check: true }) + end + end + + def check_ci_results + strong_memoize(:check_ci_results) do + ::MergeRequests::Mergeability::CheckCiStatusService.new(merge_request: merge_request, params: {}).execute + end + end + + def ci_check_failure_reason + if merge_request.actual_head_pipeline&.running? + :ci_still_running + else + check_ci_results.payload.fetch(:reason) + end + end + end + end +end diff --git a/app/services/merge_requests/mergeability/logger.rb b/app/services/merge_requests/mergeability/logger.rb new file mode 100644 index 00000000000..8b45d231e03 --- /dev/null +++ b/app/services/merge_requests/mergeability/logger.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +module MergeRequests + module Mergeability + class Logger + include Gitlab::Utils::StrongMemoize + + def initialize(merge_request:, destination: Gitlab::AppJsonLogger) + @destination = destination + @merge_request = merge_request + end + + def commit + return unless enabled? + + commit_logs + end + + def instrument(mergeability_name:) + raise ArgumentError, 'block not given' unless block_given? + + return yield unless enabled? + + op_start_db_counters = current_db_counter_payload + op_started_at = current_monotonic_time + + result = yield + + observe("mergeability.#{mergeability_name}.duration_s", current_monotonic_time - op_started_at) + + observe_sql_counters(mergeability_name, op_start_db_counters, current_db_counter_payload) + + result + end + + private + + attr_reader :destination, :merge_request + + def observe(name, value) + return unless enabled? + + observations[name.to_s].push(value) + end + + def commit_logs + attributes = Gitlab::ApplicationContext.current.merge({ + mergeability_project_id: merge_request.project.id + }) + + attributes[:mergeability_merge_request_id] = merge_request.id + attributes.merge!(observations_hash) + attributes.compact! + attributes.stringify_keys! + + destination.info(attributes) + end + + def observations_hash + transformed = observations.transform_values do |values| + next if values.empty? + + { + 'values' => values + } + end.compact + + transformed.each_with_object({}) do |key, hash| + key[1].each { |k, v| hash["#{key[0]}.#{k}"] = v } + end + end + + def observations + strong_memoize(:observations) do + Hash.new { |hash, key| hash[key] = [] } + end + end + + def observe_sql_counters(name, start_db_counters, end_db_counters) + end_db_counters.each do |key, value| + result = value - start_db_counters.fetch(key, 0) + next if result == 0 + + observe("mergeability.#{name}.#{key}", result) + end + end + + def current_db_counter_payload + ::Gitlab::Metrics::Subscribers::ActiveRecord.db_counter_payload + end + + def enabled? + strong_memoize(:enabled) do + ::Feature.enabled?(:mergeability_checks_logger, merge_request.project) + end + end + + def current_monotonic_time + ::Gitlab::Metrics::System.monotonic_time + end + end + end +end diff --git a/app/services/merge_requests/mergeability/run_checks_service.rb b/app/services/merge_requests/mergeability/run_checks_service.rb index 68f842b3322..7f205c8dd6c 100644 --- a/app/services/merge_requests/mergeability/run_checks_service.rb +++ b/app/services/merge_requests/mergeability/run_checks_service.rb @@ -15,12 +15,17 @@ module MergeRequests next if check.skip? - check_result = run_check(check) + check_result = logger.instrument(mergeability_name: check_class.to_s.demodulize.underscore) do + run_check(check) + end + result_hash << check_result break result_hash if check_result.failed? end + logger.commit + self end @@ -57,6 +62,12 @@ module MergeRequests Gitlab::MergeRequests::Mergeability::ResultsStore.new(merge_request: merge_request) end end + + def logger + strong_memoize(:logger) do + MergeRequests::Mergeability::Logger.new(merge_request: merge_request) + end + end end end end diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 5205d34baae..533d0052fb8 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -234,6 +234,7 @@ module MergeRequests end # Add comment about pushing new commits to merge requests and send nofitication emails + # def notify_about_push(merge_request) return unless @commits.present? diff --git a/app/services/merge_requests/update_assignees_service.rb b/app/services/merge_requests/update_assignees_service.rb index a6b0235c525..a13db52e34b 100644 --- a/app/services/merge_requests/update_assignees_service.rb +++ b/app/services/merge_requests/update_assignees_service.rb @@ -20,8 +20,6 @@ module MergeRequests attrs = update_attrs.merge(assignee_ids: new_ids) merge_request.update!(**attrs) - bulk_update_assignees_state(merge_request, merge_request.assignees - old_assignees) - # Defer the more expensive operations (handle_assignee_changes) to the background MergeRequests::HandleAssigneesChangeService .new(project: project, current_user: current_user) diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 0902b5195a1..6d518edc88f 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -179,18 +179,16 @@ module MergeRequests old_title_draft = MergeRequest.draft?(old_title) new_title_draft = MergeRequest.draft?(new_title) + # notify the draft status changed. Added/removed message is handled in the + # email template itself, see `change_in_merge_request_draft_status_email` template. + notify_draft_status_changed(merge_request) if old_title_draft || new_title_draft + if !old_title_draft && new_title_draft # Marked as Draft - # - merge_request_activity_counter - .track_marked_as_draft_action(user: current_user) + merge_request_activity_counter.track_marked_as_draft_action(user: current_user) elsif old_title_draft && !new_title_draft # Unmarked as Draft - # - notify_draft_status_changed(merge_request) - - merge_request_activity_counter - .track_unmarked_as_draft_action(user: current_user) + merge_request_activity_counter.track_unmarked_as_draft_action(user: current_user) end end |