Track deployed merge requests in the database

This adds tracking of merged requests when creating deployments. These
changes are hidden behind a feature flag at the moment, and no UI or
APIs have been added yet.

By tracking merge requests, developers will eventually be able to easily
see when their merge requests made it into a certain environment. This
data also allows for more accurate measuring of the time between a merge
request getting merged and deployed to a certain environment.

See https://gitlab.com/gitlab-org/gitlab/issues/32584 for more
information.
Co-authored-by: Alessio Caiazza <acaiazza@gitlab.com>

app/models/deployment.rb

@@ -10,6 +10,10 @@ class Deployment < ApplicationRecord
   belongs_to :cluster, class_name: 'Clusters::Cluster', optional: true
   belongs_to :user
   belongs_to :deployable, polymorphic: true, optional: true # rubocop:disable Cop/PolymorphicAssociations
+  has_many :deployment_merge_requests
+
+  has_many :merge_requests,
+    through: :deployment_merge_requests
 
   has_internal_id :iid, scope: :project, init: ->(s) do
     Deployment.where(project: s.project).maximum(:iid) if s&.project
@@ -149,6 +153,18 @@ class Deployment < ApplicationRecord
       project.deployments.joins(:environment)
       .where(environments: { name: self.environment.name }, ref: self.ref)
       .where.not(id: self.id)
+      .order(id: :desc)
+      .take
+  end
+
+  def previous_environment_deployment
+    project
+      .deployments
+      .success
+      .joins(:environment)
+      .where(environments: { name: environment.name })
+      .where.not(id: self.id)
+      .order(id: :desc)
       .take
   end
 
@@ -181,6 +197,18 @@ class Deployment < ApplicationRecord
     deployable&.user || user
   end
 
+  def link_merge_requests(relation)
+    select = relation.select(['merge_requests.id', id]).to_sql
+
+    # We don't use `Gitlab::Database.bulk_insert` here so that we don't need to
+    # first pluck lots of IDs into memory.
+    DeploymentMergeRequest.connection.execute(<<~SQL)
+      INSERT INTO #{DeploymentMergeRequest.table_name}
+      (merge_request_id, deployment_id)
+      #{select}
+    SQL
+  end
+
   private
 
   def ref_path

app/models/deployment_merge_request.rb

+# frozen_string_literal: true
+
+class DeploymentMergeRequest < ApplicationRecord
+  belongs_to :deployment, optional: false
+  belongs_to :merge_request, optional: false
+end

app/models/merge_request.rb

@@ -202,6 +202,9 @@ class MergeRequest < ApplicationRecord
   scope :by_commit_sha, ->(sha) do
     where('EXISTS (?)', MergeRequestDiff.select(1).where('merge_requests.latest_merge_request_diff_id = merge_request_diffs.id').by_commit_sha(sha)).reorder(nil)
   end
+  scope :by_merge_commit_sha, -> (sha) do
+    where(merge_commit_sha: sha)
+  end
   scope :join_project, -> { joins(:target_project) }
   scope :references_project, -> { references(:target_project) }
   scope :with_api_entity_associations, -> {

app/services/deployments/after_create_service.rb

@@ -33,12 +33,21 @@ module Deployments
 
         if environment.save && !environment.stopped?
           deployment.update_merge_request_metrics!
+          link_merge_requests(deployment)
         end
       end
     end
 
     private
 
+    def link_merge_requests(deployment)
+      unless Feature.enabled?(:deployment_merge_requests, deployment.project)
+        return
+      end
+
+      LinkMergeRequestsService.new(deployment).execute
+    end
+
     def environment_options
       options&.dig(:environment) || {}
     end

app/services/deployments/link_merge_requests_service.rb

+# frozen_string_literal: true
+
+module Deployments
+  # Service class for linking merge requests to deployments.
+  class LinkMergeRequestsService
+    attr_reader :deployment
+
+    # The number of commits per query for which to find merge requests.
+    COMMITS_PER_QUERY = 5_000
+
+    def initialize(deployment)
+      @deployment = deployment
+    end
+
+    def execute
+      return unless deployment.success?
+
+      if (prev = deployment.previous_environment_deployment)
+        link_merge_requests_for_range(prev.sha, deployment.sha)
+      else
+        # When no previous deployment is found we fall back to linking all merge
+        # requests merged into the deployed branch. This will not always be
+        # accurate, but it's better than having no data.
+        #
+        # We can't use the first commit in the repository as a base to compare
+        # to, as this will not scale to large repositories. For example, GitLab
+        # itself has over 150 000 commits.
+        link_all_merged_merge_requests
+      end
+    end
+
+    def link_merge_requests_for_range(from, to)
+      commits = project
+        .repository
+        .commits_between(from, to)
+        .map(&:id)
+
+      # For some projects the list of commits to deploy may be very large. To
+      # ensure we do not end up running SQL queries with thousands of WHERE IN
+      # values, we run one query per a certain number of commits.
+      #
+      # In most cases this translates to only a single query. For very large
+      # deployment we may end up running a handful of queries to get and insert
+      # the data.
+      commits.each_slice(COMMITS_PER_QUERY) do |slice|
+        merge_requests =
+          project.merge_requests.merged.by_merge_commit_sha(slice)
+
+        deployment.link_merge_requests(merge_requests)
+      end
+    end
+
+    def link_all_merged_merge_requests
+      merge_requests =
+        project.merge_requests.merged.by_target_branch(deployment.ref)
+
+      deployment.link_merge_requests(merge_requests)
+    end
+
+    private
+
+    def project
+      deployment.project
+    end
+  end
+end

app/services/deployments/update_service.rb

@@ -10,7 +10,22 @@ module Deployments
     end
 
     def execute
-      deployment.update(status: params[:status])
+      # A regular update() does not trigger the state machine transitions, which
+      # we need to ensure merge requests are linked when changing the status to
+      # success. To work around this we use this case statment, using the right
+      # event methods to trigger the transition hooks.
+      case params[:status]
+      when 'running'
+        deployment.run
+      when 'success'
+        deployment.succeed
+      when 'failed'
+        deployment.drop
+      when 'canceled'
+        deployment.cancel
+      else
+        false
+      end
     end
   end
 end

changelogs/unreleased/deployment-commit-tracking.yml

+---
+title: Add deployment_merge_requests table
+merge_request: 18755
+author:
+type: other

db/migrate/20191017134513_add_deployment_merge_requests.rb

+# frozen_string_literal: true
+
+class AddDeploymentMergeRequests < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :deployment_merge_requests, id: false do |t|
+      t.references(
+        :deployment,
+        foreign_key: { on_delete: :cascade },
+        type: :integer,
+        index: false,
+        null: false
+      )
+
+      t.references(
+        :merge_request,
+        foreign_key: { on_delete: :cascade },
+        type: :integer,
+        index: true,
+        null: false
+      )
+
+      t.index(
+        [:deployment_id, :merge_request_id],
+        unique: true,
+        name: 'idx_deployment_merge_requests_unique_index'
+      )
+    end
+  end
+end

db/schema.rb

@@ -1257,6 +1257,13 @@ ActiveRecord::Schema.define(version: 2019_10_26_124116) do
     t.index ["token_encrypted"], name: "index_deploy_tokens_on_token_encrypted", unique: true
   end
 
+  create_table "deployment_merge_requests", id: false, force: :cascade do |t|
+    t.integer "deployment_id", null: false
+    t.integer "merge_request_id", null: false
+    t.index ["deployment_id", "merge_request_id"], name: "idx_deployment_merge_requests_unique_index", unique: true
+    t.index ["merge_request_id"], name: "index_deployment_merge_requests_on_merge_request_id"
+  end
+
   create_table "deployments", id: :serial, force: :cascade do |t|
     t.integer "iid", null: false
     t.integer "project_id", null: false
@@ -4168,6 +4175,8 @@ ActiveRecord::Schema.define(version: 2019_10_26_124116) do
   add_foreign_key "dependency_proxy_blobs", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "dependency_proxy_group_settings", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "deploy_keys_projects", "projects", name: "fk_58a901ca7e", on_delete: :cascade
+  add_foreign_key "deployment_merge_requests", "deployments", on_delete: :cascade
+  add_foreign_key "deployment_merge_requests", "merge_requests", on_delete: :cascade
   add_foreign_key "deployments", "clusters", name: "fk_289bba3222", on_delete: :nullify
   add_foreign_key "deployments", "projects", name: "fk_b9a3851b82", on_delete: :cascade
   add_foreign_key "description_versions", "epics", on_delete: :cascade

lib/gitlab/data_builder/deployment.rb

@@ -6,11 +6,17 @@ module Gitlab
       extend self
 
       def build(deployment)
+        # Deployments will not have a deployable when created using the API.
+        deployable_url =
+          if deployment.deployable
+            Gitlab::UrlBuilder.build(deployment.deployable)
+          end
+
         {
           object_kind: 'deployment',
           status: deployment.status,
           deployable_id: deployment.deployable_id,
-          deployable_url: Gitlab::UrlBuilder.build(deployment.deployable),
+          deployable_url: deployable_url,
           environment: deployment.environment.name,
           project: deployment.project.hook_attrs,
           short_sha: deployment.short_sha,

spec/lib/gitlab/data_builder/deployment_spec.rb

@@ -35,5 +35,12 @@ describe Gitlab::DataBuilder::Deployment do
       expect(data[:commit_url]).to eq(expected_commit_url)
       expect(data[:commit_title]).to eq(commit.title)
     end
+
+    it 'does not include the deployable URL when there is no deployable' do
+      deployment = create(:deployment, status: :failed, deployable: nil)
+      data = described_class.build(deployment)
+
+      expect(data[:deployable_url]).to be_nil
+    end
   end
 end

spec/models/deployment_merge_request_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DeploymentMergeRequest do
+  let(:mr) { create(:merge_request, :merged) }
+  let(:deployment) { create(:deployment, :success, project: project) }
+  let(:project) { mr.project }
+
+  subject { described_class.new(deployment: deployment, merge_request: mr) }
+
+  it { is_expected.to belong_to(:deployment).required }
+  it { is_expected.to belong_to(:merge_request).required }
+end

spec/models/deployment_spec.rb

@@ -10,6 +10,8 @@ describe Deployment do
   it { is_expected.to belong_to(:cluster).class_name('Clusters::Cluster') }
   it { is_expected.to belong_to(:user) }
   it { is_expected.to belong_to(:deployable) }
+  it { is_expected.to have_many(:deployment_merge_requests) }
+  it { is_expected.to have_many(:merge_requests).through(:deployment_merge_requests) }
 
   it { is_expected.to delegate_method(:name).to(:environment).with_prefix }
   it { is_expected.to delegate_method(:commit).to(:project) }
@@ -361,4 +363,82 @@ describe Deployment do
         .to raise_error(ActiveRecord::RecordNotFound)
     end
   end
+
+  describe '#previous_deployment' do
+    it 'returns the previous deployment' do
+      deploy1 = create(:deployment)
+      deploy2 = create(
+        :deployment,
+        project: deploy1.project,
+        environment: deploy1.environment
+      )
+
+      expect(deploy2.previous_deployment).to eq(deploy1)
+    end
+  end
+
+  describe '#link_merge_requests' do
+    it 'links merge requests with a deployment' do
+      deploy = create(:deployment)
+      mr1 = create(
+        :merge_request,
+        :merged,
+        target_project: deploy.project,
+        source_project: deploy.project
+      )
+
+      mr2 = create(
+        :merge_request,
+        :merged,
+        target_project: deploy.project,
+        source_project: deploy.project
+      )
+
+      deploy.link_merge_requests(deploy.project.merge_requests)
+
+      expect(deploy.merge_requests).to include(mr1, mr2)
+    end
+  end
+
+  describe '#previous_environment_deployment' do
+    it 'returns the previous deployment of the same environment' do
+      deploy1 = create(:deployment, :success, ref: 'v1.0.0')
+      deploy2 = create(
+        :deployment,
+        :success,
+        project: deploy1.project,
+        environment: deploy1.environment,
+        ref: 'v1.0.1'
+      )
+
+      expect(deploy2.previous_environment_deployment).to eq(deploy1)
+    end
+
+    it 'ignores deployments that were not successful' do
+      deploy1 = create(:deployment, :failed, ref: 'v1.0.0')
+      deploy2 = create(
+        :deployment,
+        :success,
+        project: deploy1.project,
+        environment: deploy1.environment,
+        ref: 'v1.0.1'
+      )
+
+      expect(deploy2.previous_environment_deployment).to be_nil
+    end
+
+    it 'ignores deployments for different environments' do
+      deploy1 = create(:deployment, :success, ref: 'v1.0.0')
+      preprod = create(:environment, project: deploy1.project, name: 'preprod')
+      deploy2 = create(
+        :deployment,
+        :success,
+        project: deploy1.project,
+        environment: preprod,
+        ref: 'v1.0.1'
+      )
+
+      expect(deploy2.previous_environment_deployment).to be_nil
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -283,6 +283,16 @@ describe MergeRequest do
     end
   end
 
+  describe '.by_merge_commit_sha' do
+    it 'returns merge requests that match the given merge commit' do
+      mr = create(:merge_request, :merged, merge_commit_sha: '123abc')
+
+      create(:merge_request, :merged, merge_commit_sha: '123def')
+
+      expect(described_class.by_merge_commit_sha('123abc')).to eq([mr])
+    end
+  end
+
   describe '.in_projects' do
     it 'returns the merge requests for a set of projects' do
       expect(described_class.in_projects(Project.all)).to eq([subject])

spec/requests/api/deployments_spec.rb

@@ -137,14 +137,42 @@ describe API::Deployments do
 
         expect(response).to have_gitlab_http_status(500)
       end
+
+      it 'links any merged merge requests to the deployment' do
+        mr = create(
+          :merge_request,
+          :merged,
+          target_project: project,
+          source_project: project,
+          target_branch: 'master',
+          source_branch: 'foo'
+        )
+
+        post(
+          api("/projects/#{project.id}/deployments", user),
+          params: {
+            environment: 'production',
+            sha: sha,
+            ref: 'master',
+            tag: false,
+            status: 'success'
+          }
+        )
+
+        deploy = project.deployments.last
+
+        expect(deploy.merge_requests).to eq([mr])
+      end
     end
 
     context 'as a developer' do
-      it 'creates a new deployment' do
-        developer = create(:user)
+      let(:developer) { create(:user) }
 
+      before do
         project.add_developer(developer)
+      end
 
+      it 'creates a new deployment' do
         post(
           api("/projects/#{project.id}/deployments", developer),
           params: {
@@ -161,6 +189,32 @@ describe API::Deployments do
         expect(json_response['sha']).to eq(sha)
         expect(json_response['ref']).to eq('master')
       end
+
+      it 'links any merged merge requests to the deployment' do
+        mr = create(
+          :merge_request,
+          :merged,
+          target_project: project,
+          source_project: project,
+          target_branch: 'master',
+          source_branch: 'foo'
+        )
+
+        post(
+          api("/projects/#{project.id}/deployments", developer),
+          params: {
+            environment: 'production',
+            sha: sha,
+            ref: 'master',
+            tag: false,
+            status: 'success'
+          }
+        )
+
+        deploy = project.deployments.last
+
+        expect(deploy.merge_requests).to eq([mr])
+      end
     end
 
     context 'as non member' do
@@ -182,7 +236,7 @@ describe API::Deployments do
   end
 
   describe 'PUT /projects/:id/deployments/:deployment_id' do
-    let(:project) { create(:project) }
+    let(:project) { create(:project, :repository) }
     let(:build) { create(:ci_build, :failed, project: project) }
     let(:environment) { create(:environment, project: project) }
     let(:deploy) do
@@ -191,7 +245,8 @@ describe API::Deployments do
         :failed,
         project: project,
         environment: environment,
-        deployable: nil
+        deployable: nil,
+        sha: project.commit.sha
       )
     end
 
@@ -216,6 +271,26 @@ describe API::Deployments do
         expect(response).to have_gitlab_http_status(200)
         expect(json_response['status']).to eq('success')
       end
+
+      it 'links merge requests when the deployment status changes to success', :sidekiq_inline do
+        mr = create(
+          :merge_request,
+          :merged,
+          target_project: project,
+          source_project: project,
+          target_branch: 'master',
+          source_branch: 'foo'
+        )
+
+        put(
+          api("/projects/#{project.id}/deployments/#{deploy.id}", user),
+          params: { status: 'success' }
+        )
+
+        deploy = project.deployments.last
+
+        expect(deploy.merge_requests).to eq([mr])
+      end
     end
 
     context 'as a developer' do

spec/services/deployments/after_create_service_spec.rb

@@ -53,6 +53,14 @@ describe Deployments::AfterCreateService do
       service.execute
     end
 
+    it 'links merge requests to deployment' do
+      expect_next_instance_of(Deployments::LinkMergeRequestsService, deployment) do |link_mr_service|
+        expect(link_mr_service).to receive(:execute)
+      end
+
+      service.execute
+    end
+
     it 'returns the deployment' do
       expect(subject.execute).to eq(deployment)
     end
@@ -237,4 +245,30 @@ describe Deployments::AfterCreateService do
       end
     end
   end
+
+  describe '#update_environment' do
+    it 'links the merge requests' do
+      double = instance_double(Deployments::LinkMergeRequestsService)
+
+      allow(Deployments::LinkMergeRequestsService)
+        .to receive(:new)
+        .with(deployment)
+        .and_return(double)
+
+      expect(double).to receive(:execute)
+
+      service.update_environment(deployment)
+    end
+
+    context 'when the tracking of merge requests is disabled' do
+      it 'does nothing' do
+        stub_feature_flags(deployment_merge_requests: false)
+
+        expect(Deployments::LinkMergeRequestsService)
+          .not_to receive(:new)
+
+        service.update_environment(deployment)
+      end
+    end
+  end
 end

spec/services/deployments/link_merge_requests_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Deployments::LinkMergeRequestsService do
+  describe '#execute' do
+    context 'when the deployment did not succeed' do
+      it 'does nothing' do
+        deploy = create(:deployment, :failed)
+
+        expect(deploy).not_to receive(:link_merge_requests)
+
+        described_class.new(deploy).execute
+      end
+    end
+
+    context 'when there is a previous deployment' do
+      it 'links all merge requests merged since the previous deployment' do
+        deploy1 = create(:deployment, :success, sha: 'foo')
+        deploy2 = create(
+          :deployment,
+          :success,
+          sha: 'bar',
+          project: deploy1.project,
+          environment: deploy1.environment
+        )
+
+        service = described_class.new(deploy2)
+
+        expect(service)
+          .to receive(:link_merge_requests_for_range)
+          .with('foo', 'bar')
+
+        service.execute
+      end
+    end
+
+    context 'when there are no previous deployments' do
+      it 'links all merged merge requests' do
+        deploy = create(:deployment, :success)
+        service = described_class.new(deploy)
+
+        expect(service).to receive(:link_all_merged_merge_requests)
+
+        service.execute
+      end
+    end
+  end
+
+  describe '#link_merge_requests_for_range' do
+    it 'links merge requests' do
+      project = create(:project, :repository)
+      environment = create(:environment, project: project)
+      deploy =
+        create(:deployment, :success, project: project, environment: environment)
+
+      mr1 = create(
+        :merge_request,
+        :merged,
+        merge_commit_sha: '1e292f8fedd741b75372e19097c76d327140c312',
+        source_project: project,
+        target_project: project
+      )
+
+      mr2 = create(
+        :merge_request,
+        :merged,
+        merge_commit_sha: '2d1db523e11e777e49377cfb22d368deec3f0793',
+        source_project: project,
+        target_project: project
+      )
+
+      described_class.new(deploy).link_merge_requests_for_range(
+        '7975be0116940bf2ad4321f79d02a55c5f7779aa',
+        'ddd0f15ae83993f5cb66a927a28673882e99100b'
+      )
+
+      expect(deploy.merge_requests).to include(mr1, mr2)
+    end
+  end
+
+  describe '#link_all_merged_merge_requests' do
+    it 'links all merged merge requests targeting the deployed branch' do
+      project = create(:project, :repository)
+      environment = create(:environment, project: project)
+      deploy =
+        create(:deployment, :success, project: project, environment: environment)
+
+      mr1 = create(
+        :merge_request,
+        :merged,
+        source_project: project,
+        target_project: project,
+        source_branch: 'source1',
+        target_branch: deploy.ref
+      )
+
+      mr2 = create(
+        :merge_request,
+        :merged,
+        source_project: project,
+        target_project: project,
+        source_branch: 'source2',
+        target_branch: deploy.ref
+      )
+
+      mr3 = create(
+        :merge_request,
+        :merged,
+        source_project: project,
+        target_project: project,
+        target_branch: 'foo'
+      )
+
+      described_class.new(deploy).link_all_merged_merge_requests
+
+      expect(deploy.merge_requests).to include(mr1, mr2)
+      expect(deploy.merge_requests).not_to include(mr3)
+    end
+  end
+end

spec/services/deployments/update_service_spec.rb

@@ -3,13 +3,55 @@
 require 'spec_helper'
 
 describe Deployments::UpdateService do
-  let(:deploy) { create(:deployment, :running) }
-  let(:service) { described_class.new(deploy, status: 'success') }
+  let(:deploy) { create(:deployment) }
 
   describe '#execute' do
-    it 'updates the status of a deployment' do
-      expect(service.execute).to eq(true)
-      expect(deploy.status).to eq('success')
+    it 'can update the status to running' do
+      expect(described_class.new(deploy, status: 'running').execute)
+        .to be_truthy
+
+      expect(deploy).to be_running
+    end
+
+    it 'can update the status to success' do
+      expect(described_class.new(deploy, status: 'success').execute)
+        .to be_truthy
+
+      expect(deploy).to be_success
+    end
+
+    it 'can update the status to failed' do
+      expect(described_class.new(deploy, status: 'failed').execute)
+        .to be_truthy
+
+      expect(deploy).to be_failed
+    end
+
+    it 'can update the status to canceled' do
+      expect(described_class.new(deploy, status: 'canceled').execute)
+        .to be_truthy
+
+      expect(deploy).to be_canceled
+    end
+
+    it 'returns false when the status is not supported' do
+      expect(described_class.new(deploy, status: 'kittens').execute)
+        .to be_falsey
+    end
+
+    it 'links merge requests when changing the status to success', :sidekiq_inline do
+      mr = create(
+        :merge_request,
+        :merged,
+        target_project: deploy.project,
+        source_project: deploy.project,
+        target_branch: 'master',
+        source_branch: 'foo'
+      )
+
+      described_class.new(deploy, status: 'success').execute
+
+      expect(deploy.merge_requests).to eq([mr])
     end
   end
 end