Code Review Usage Ping for Create SMAU

Nick Thomas authored 5 years ago and Mayra Cabrera committed 5 years ago

Prior to 12.1, rebase status was looked up directly from Gitaly. In
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/14417 , a DB
column was added to track the status instead. However, we couldn't stop
looking at the gitaly status immediately, since some rebases may been
running across the upgrade.

Now that we're in 12.3, it is safe to remove the direct-to-gitaly
lookup. This also happens to fix a 500 error that is seen when viewing
an MR for a fork where the source project has been removed.

We still look at the Gitaly status in the service, just in case Gitaly
and Sidekiq get out of sync - I assume this is possible, and it's a
relatively cheap check.

Since we atomically check and set `merge_requests.rebase_jid`, we
should never enqueue two `RebaseWorker` jobs in parallel.

Concurrent calls to UserMergeToRef RPC updating a single ref
can lead to an opaque fail that is being rescued at Gitaly.

So this commit adds an exclusive lease to the mergeability
check process with the key as the current MR ID.

MergeRequests::PushOptionsHandlerService has been updated to allow
creating and updating merge requests with the `title` and
`description` set using git push options.

To create a new merge request and set its title and description:

      git push -u origin -o merge_request.create \
        -o merge_request.title="My title" \
        -o merge_request.description="My description"

To update an existing merge request and set its title and
description:

      git push -u origin -o merge_request.title="My title" \
        -o merge_request.description="My description"

Issue https://gitlab.com/gitlab-org/gitlab-ce/issues/64320

Reusing the existing `IssuableBaseService#filter_params` which uses
the policies to determine what params a user can set, and which values
it can be set to.

This also removed the need for the seperate call to
`IssuableBaseService#ensure_milestone_available`.

The `Issues::BuildService` does not suffer from this because it limits
the params that are assignable to the `title`, `description` and
`milestone_id`.

Let's move shared code between create_params and update_params
into a new base_params.

update_params becomes very thin, but it still may be clearer that
the params are being shared if we have a method called
base_params, rather than have create_params merge in
update_params.

MergeRequests::PushOptionsHandlerService has been updated to allow
creating and updating merge requests with the
`remove_source_branch` set using git push options.

To create a new merge request and set it to remove the source branch
when it is merged:

  git push -u origin -o merge_request.create \
    -o merge_request.remove_source_branch

To update an existing merge request and set it to remove the source
branch when it is merged:

  git push -u origin -o merge_request.remove_source_branch

Issue https://gitlab.com/gitlab-org/gitlab-ce/issues/64320

Suggests to use a JSON structured log instead

Related to https://gitlab.com/gitlab-org/gitlab-ce/issues/54102

Turns out push rules to validate commit message does
not apply in the context of automatic merge to
the refs/merge-requests/:iid/merge. Mainly because
if it fails to merge to it, we currently can't give enough
preemptive feedback to the user and it'll turn the
merge request unmergeable (given we automatically
mark it as unmergeable if we can't merge to the ref).

In general, it's a systematic operation, which already
bypasses user authorization and git hooks.

Therefore, this commit makes it bypass the push rules
at EE as well.

Currently, MergeToRefService is specifically designed for
createing merge commits from source branch and target branch of
merge reqeusts. We extend this behavior to source branch and any
target ref paths.

Create explicit endpoint - abort.

This MR introduces tracking of the `rebase_jid` for merge requests. As
with `merge_ongoing?`, `rebase_in_progress?` will now return true if a
rebase is proceeding in sidekiq.

After one release, we should remove the Gitaly-based lookup of rebases.
It is much better to track this kind of thing via the database.

When `confidential_issue_project_id` is set and the issue is
under that project, create the a note about branch creation
in that project. If not, do nothing.

When creating `new_merge_request` system note, set the project
where the MR will be referenced from so it'll be linked to when
the MR is created in another project.

When recheck flag is true, we make sure the merge-ref
is indeed outdated. If it is, we update it along
the merge status.

This couples the code that transitions the `MergeRequest#merge_status`
and refs/merge-requests/:iid/merge ref update.

In general, instead of directly telling `MergeToRefService` to update
the merge ref, we should rely on `MergeabilityCheckService` to keep
both the merge status and merge ref synced. Now, if the merge_status is
`can_be_merged` it means the merge-ref is also updated to the latest.

We've also updated the logic to be more systematic and less user-based.

When target branch is updated, Auto Merge should be canceled.

We don't have a way to create MR pipeline today, this is one
of the headaches for users.
This change is a preliminary work for the capability.

We should cancel auto merge when merge request is closed.

We have one auto merge strategy today - Merge When Pipeline
Succeeds.

In order to add more strategies for Merge Train feature,
we abstract the architecture to be more extensible.

Removed arguments

Fix spec

This introduces payload to the ServiceResponse with
the merge ref HEAD commit data

This couples the code that transitions the `MergeRequest#merge_status`
and refs/merge-requests/:iid/merge ref update.

In general, instead of directly telling `MergeToRefService` to update
the merge ref, we should rely on `MergeabilityCheckService` to keep
both the merge status and merge ref synced. Now, if the merge_status is
`can_be_merged` it means the merge-ref is also updated to the latest.

We've also updated the logic to be more systematic and less user-based.

Refactor

Luke Duncalfe authored 5 years ago and Douwe Maan committed 5 years ago

The new two-step Gitaly `Rebase` RPC yields the rebase commit SHA to the
client before proceeding with the rebase.

This avoids an issue where the rebase commit SHA was returned when the
RPC had fully completed, and in some cases this would be after the Rails
`post_receive` worker services had already run. In these situations,
the merge request did not yet have its rebase_commit_sha attribute set
introducing the possibility for bugs (such as previous approvals being
reset).

https://gitlab.com/gitlab-org/gitlab-ee/issues/5966

Due to the nature of pipelines for merge requests, deployments.ref can
be a merge request ref instead of a branch name.

We support the environment auto-stop hook for this case

Users attempting to set merge requests to `remove_source_branch` to
`false` would encounter an Error 500 because the UpdateService and API
checked `present?`, which would always return `false`. We now just
use `has_key?` to decide whether the parameter is present.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/60530

- Externalize strings in milestones_helper
- Externalize strings in app/services
- Update PO file

With multiple MR assignees, `merge_request.assignee_id` is always nil,
and `merge_request.assignee_ids` isn't in `merge_request.attributes`.
So the existing code doesn't set assignees in the created MR.

This fix gets all the tests passing, but we should also check that no
other associations in the MergeRequest need similar fixups.

To make sure all known issues are linked to the correct epic, I've gone
through the code base, and updated the comments where required.

This will ensure that now and in the future, PushOptionsHandlerService
will not cause the post_receive API endpoint from running other code if
something causes an unknown exception.

Exceptions are no longer raised, instead all errors encountered are
added to the errors property.

MergeRequests::BuildService is used to generate attributes of a new
merge request.

Code moved from Api::Internal to Api::Helpers::InternalHelpers.

MergeRequests::PushOptionsHandlerService has been updated to allow
creating and updating merge requests with the
`merge_when_pipeline_succeeds` set using git push options.

To create a new merge request and set it to merge when the pipeline
succeeds:

  git push -u origin -o merge_request.create \
    -o merge_request.merge_when_pipeline_succeeds

To update an existing merge request and set it to merge when the
pipeline succeeds:

  git push -u origin -o merge_request.merge_when_pipeline_succeeds

Issue https://gitlab.com/gitlab-org/gitlab-ce/issues/53198

Backports https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/10161
(code out of ee/ folder).

To create a new merge request:

  git push -u origin -o merge_request.create

To create a new merge request setting target branch:

  git push -u origin -o merge_request.create \
    -o merge_request.target=123

To update an existing merge request with a new target branch:

  git push -u origin -o merge_request.target=123

A new Gitlab::PushOptions class handles parsing and validating the push
options array. This can be the start of the standard of GitLab accepting
push options that follow namespacing rules. Rules are discussed in issue
https://gitlab.com/gitlab-org/gitlab-ce/issues/43263.

E.g. these push options:

  -o merge_request.create -o merge_request.target=123

Become parsed as:

  {
    merge_request: {
      create: true,
      target: '123',
    }
  }

And are fetched with the class via:

  push_options.get(:merge_request)
  push_options.get(:merge_request, :create)
  push_options.get(:merge_request, :target)

A new MergeRequests::PushOptionsHandlerService takes the `merge_request`
namespaced push options and handles creating and updating
merge requests.

Any errors encountered are passed to the existing `output` Hash in
Api::Internal's `post_receive` endpoint, and passed to gitlab-shell
where they're output to the user.

Issue https://gitlab.com/gitlab-org/gitlab-ce/issues/43263

Currently, pipelines for merge requests are triggered when source or
target branch is updated. However, we should create only when source
branch is updated, because it runs unexpected pipelines.

MWPSService currently uses the old pipeline lookup method. It searches
related merge requests with pipeline.ref, however, this doesn't work
for attached/detached merge request pipelines.