Merge branch 'deployment-commit-tracking' into 'master'

Track deployed merge requests in the database

Closes #32584

See merge request gitlab-org/gitlab!18755

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

@@ -205,6 +205,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_11_05_094625) 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
@@ -4191,6 +4198,8 @@ ActiveRecord::Schema.define(version: 2019_11_05_094625) 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