Merge branch 'feature/environment-teardown-when-branch-deleted' into 'master'

app/models/environment.rb

@@ -37,6 +37,10 @@ class Environment < ActiveRecord::Base
     state :stopped
   end
 
+  def recently_updated_on_branch?(ref)
+    ref.to_s == last_deployment.try(:ref)
+  end
+
   def last_deployment
     deployments.last
   end
@@ -92,6 +96,7 @@ class Environment < ActiveRecord::Base
   def stop!(current_user)
     return unless stoppable?
 
+    stop
     stop_action.play(current_user)
   end
 end

app/models/merge_request.rb

@@ -692,12 +692,15 @@ class MergeRequest < ActiveRecord::Base
   def environments
     return [] unless diff_head_commit
 
-    @environments ||=
-      begin
-        envs = target_project.environments_for(target_branch, diff_head_commit, with_tags: true)
-        envs.concat(source_project.environments_for(source_branch, diff_head_commit)) if source_project
-        envs.uniq
-      end
+    @environments ||= begin
+      target_envs = target_project.environments_for(
+        target_branch, commit: diff_head_commit, with_tags: true)
+
+      source_envs = source_project.environments_for(
+        source_branch, commit: diff_head_commit) if source_project
+
+      (target_envs.to_a + source_envs.to_a).uniq
+    end
   end
 
   def state_human_name

app/models/project.rb

@@ -1323,22 +1323,30 @@ class Project < ActiveRecord::Base
     Gitlab::Redis.with { |redis| redis.del(pushes_since_gc_redis_key) }
   end
 
-  def environments_for(ref, commit, with_tags: false)
-    environment_ids = deployments.group(:environment_id).
-      select(:environment_id)
+  def environments_for(ref, commit: nil, with_tags: false)
+    deployments_query = with_tags ? 'ref = ? OR tag IS TRUE' : 'ref = ?'
 
-    environment_ids =
-      if with_tags
-        environment_ids.where('ref=? OR tag IS TRUE', ref)
-      else
-        environment_ids.where(ref: ref)
-      end
+    environment_ids = deployments
+      .where(deployments_query, ref.to_s)
+      .group(:environment_id)
+      .select(:environment_id)
+
+    environments_found = environments.available
+      .where(id: environment_ids).to_a
+
+    return environments_found unless commit
 
-    environments.available.where(id: environment_ids).select do |environment|
+    environments_found.select do |environment|
       environment.includes_commit?(commit)
     end
   end
 
+  def environments_recently_updated_on_branch(branch)
+    environments_for(branch).select do |environment|
+      environment.recently_updated_on_branch?(branch)
+    end
+  end
+
   private
 
   def pushes_since_gc_redis_key

app/services/after_branch_delete_service.rb

+require_relative 'base_service'
+
+##
+# Branch can be deleted either by DeleteBranchService
+# or by GitPushService.
+#
+class AfterBranchDeleteService < BaseService
+  attr_reader :branch_name
+
+  def execute(branch_name)
+    @branch_name = branch_name
+
+    stop_environments
+  end
+
+  private
+
+  def stop_environments
+    Ci::StopEnvironmentsService
+      .new(project, current_user)
+      .execute(branch_name)
+  end
+end

app/services/ci/stop_environments_service.rb

+module Ci
+  class StopEnvironmentsService < BaseService
+    attr_reader :ref
+
+    def execute(branch_name)
+      @ref = branch_name
+
+      return unless has_ref?
+
+      environments.each do |environment|
+        next unless environment.stoppable?
+        next unless can?(current_user, :create_deployment, project)
+
+        environment.stop!(current_user)
+      end
+    end
+
+    private
+
+    def has_ref?
+      @ref.present?
+    end
+
+    def environments
+      @environments ||= project
+        .environments_recently_updated_on_branch(@ref)
+    end
+  end
+end

app/services/git_push_service.rb

@@ -49,10 +49,7 @@ class GitPushService < BaseService
       update_gitattributes if is_default_branch?
     end
 
-    # Update merge requests that may be affected by this push. A new branch
-    # could cause the last commit of a merge request to change.
-    update_merge_requests
-
+    execute_related_hooks
     perform_housekeeping
   end
 
@@ -62,14 +59,24 @@ class GitPushService < BaseService
 
   protected
 
-  def update_merge_requests
-    UpdateMergeRequestsWorker.perform_async(@project.id, current_user.id, params[:oldrev], params[:newrev], params[:ref])
+  def execute_related_hooks
+    # Update merge requests that may be affected by this push. A new branch
+    # could cause the last commit of a merge request to change.
+    #
+    UpdateMergeRequestsWorker
+      .perform_async(@project.id, current_user.id, params[:oldrev], params[:newrev], params[:ref])
 
     EventCreateService.new.push(@project, current_user, build_push_data)
     @project.execute_hooks(build_push_data.dup, :push_hooks)
     @project.execute_services(build_push_data.dup, :push_hooks)
     Ci::CreatePipelineService.new(@project, current_user, build_push_data).execute
     ProjectCacheWorker.perform_async(@project.id)
+
+    if push_remove_branch?
+      AfterBranchDeleteService
+        .new(project, current_user)
+        .execute(branch_name)
+    end
   end
 
   def perform_housekeeping

changelogs/unreleased/feature-environment-teardown-when-branch-deleted.yml

+---
+title: Auto-close environment when branch is deleted
+merge_request: 7355
+author: 

spec/factories/deployments.rb

@@ -3,8 +3,9 @@ FactoryGirl.define do
     sha '97de212e80737a608d939f648d959671fb0a0142'
     ref 'master'
     tag false
+    user
     project nil
-
+    deployable factory: :ci_build
     environment factory: :environment
 
     after(:build) do |deployment, evaluator|

spec/factories/environments.rb

@@ -4,5 +4,33 @@ FactoryGirl.define do
 
     project factory: :empty_project
     sequence(:external_url) { |n| "https://env#{n}.example.gitlab.com" }
+
+    trait :with_review_app do |environment|
+      project
+
+      transient do
+        ref 'master'
+      end
+
+      # At this point `review app` is an ephemeral concept related to
+      # deployments being deployed for given environment. There is no
+      # first-class `review app` available so we need to create set of
+      # interconnected objects to simulate a review app.
+      #
+      after(:create) do |environment, evaluator|
+        deployment = create(:deployment,
+                            environment: environment,
+                            project: environment.project,
+                            ref: evaluator.ref,
+                            sha: environment.project.commit(evaluator.ref).id)
+
+        teardown_build = create(:ci_build, :manual,
+                                name: "#{deployment.environment.name}:teardown",
+                                pipeline: deployment.deployable.pipeline)
+
+        deployment.update_column(:on_stop, teardown_build.name)
+        environment.update_attribute(:deployments, [deployment])
+      end
+    end
   end
 end

spec/features/environments_spec.rb

@@ -6,8 +6,8 @@ feature 'Environments', feature: true do
   given(:role) { :developer }
 
   background do
-    login_as(user)
     project.team << [user, role]
+    login_as(user)
   end
 
   describe 'when showing environments' do
@@ -16,7 +16,7 @@ feature 'Environments', feature: true do
     given!(:manual) { }
 
     before do
-      visit namespace_project_environments_path(project.namespace, project)
+      visit_environments(project)
     end
 
     context 'shows two tabs' do
@@ -142,7 +142,7 @@ feature 'Environments', feature: true do
     given!(:manual) { }
 
     before do
-      visit namespace_project_environment_path(project.namespace, project, environment)
+      visit_environment(environment)
     end
 
     context 'without deployments' do
@@ -152,7 +152,9 @@ feature 'Environments', feature: true do
     end
 
     context 'with deployments' do
-      given(:deployment) { create(:deployment, environment: environment) }
+      given(:deployment) do
+        create(:deployment, environment: environment, deployable: nil)
+      end
 
       scenario 'does show deployment SHA' do
         expect(page).to have_link(deployment.short_sha)
@@ -232,7 +234,7 @@ feature 'Environments', feature: true do
 
   describe 'when creating a new environment' do
     before do
-      visit namespace_project_environments_path(project.namespace, project)
+      visit_environments(project)
     end
 
     context 'when logged as developer' do
@@ -271,4 +273,56 @@ feature 'Environments', feature: true do
       end
     end
   end
+
+  feature 'auto-close environment when branch deleted' do
+    given(:project) { create(:project) }
+
+    given!(:environment) do
+      create(:environment, :with_review_app, project: project,
+                                             ref: 'feature')
+    end
+
+    scenario 'user visits environment page' do
+      visit_environment(environment)
+
+      expect(page).to have_link('Stop')
+    end
+
+    scenario 'user deletes the branch with running environment' do
+      visit namespace_project_branches_path(project.namespace, project)
+
+      remove_branch_with_hooks(project, user, 'feature') do
+        page.within('.js-branch-feature') { find('a.btn-remove').click }
+      end
+
+      visit_environment(environment)
+
+      expect(page).to have_no_link('Stop')
+    end
+
+    ##
+    # This is a workaround for problem described in #24543
+    #
+    def remove_branch_with_hooks(project, user, branch)
+      params = {
+        oldrev: project.commit(branch).id,
+        newrev: Gitlab::Git::BLANK_SHA,
+        ref: "refs/heads/#{branch}"
+      }
+
+      yield
+
+      GitPushService.new(project, user, params).execute
+    end
+  end
+
+  def visit_environments(project)
+    visit namespace_project_environments_path(project.namespace, project)
+  end
+
+  def visit_environment(environment)
+    visit namespace_project_environment_path(environment.project.namespace,
+                                             environment.project,
+                                             environment)
+  end
 end

spec/models/environment_spec.rb

@@ -166,4 +166,25 @@ describe Environment, models: true do
       end
     end
   end
+
+  describe 'recently_updated_on_branch?' do
+    subject { environment.recently_updated_on_branch?('feature') }
+
+    context 'when last deployment to environment is the most recent one' do
+      before do
+        create(:deployment, environment: environment, ref: 'feature')
+      end
+
+      it { is_expected.to be true }
+    end
+
+    context 'when last deployment to environment is not the most recent' do
+      before do
+        create(:deployment, environment: environment, ref: 'feature')
+        create(:deployment, environment: environment, ref: 'master')
+      end
+
+      it { is_expected.to be false }
+    end
+  end
 end

spec/models/project_spec.rb

@@ -1640,15 +1640,18 @@ describe Project, models: true do
       end
 
       it 'returns environment when with_tags is set' do
-        expect(project.environments_for('master', project.commit, with_tags: true)).to contain_exactly(environment)
+        expect(project.environments_for('master', commit: project.commit, with_tags: true))
+          .to contain_exactly(environment)
       end
 
       it 'does not return environment when no with_tags is set' do
-        expect(project.environments_for('master', project.commit)).to be_empty
+        expect(project.environments_for('master', commit: project.commit))
+          .to be_empty
       end
 
       it 'does not return environment when commit is not part of deployment' do
-        expect(project.environments_for('master', project.commit('feature'))).to be_empty
+        expect(project.environments_for('master', commit: project.commit('feature')))
+          .to be_empty
       end
     end
 
@@ -1658,15 +1661,65 @@ describe Project, models: true do
       end
 
       it 'returns environment when ref is set' do
-        expect(project.environments_for('master', project.commit)).to contain_exactly(environment)
+        expect(project.environments_for('master', commit: project.commit))
+          .to contain_exactly(environment)
       end
 
       it 'does not environment when ref is different' do
-        expect(project.environments_for('feature', project.commit)).to be_empty
+        expect(project.environments_for('feature', commit: project.commit))
+          .to be_empty
       end
 
       it 'does not return environment when commit is not part of deployment' do
-        expect(project.environments_for('master', project.commit('feature'))).to be_empty
+        expect(project.environments_for('master', commit: project.commit('feature')))
+          .to be_empty
+      end
+
+      it 'returns environment when commit constraint is not set' do
+        expect(project.environments_for('master'))
+          .to contain_exactly(environment)
+      end
+    end
+  end
+
+  describe '#environments_recently_updated_on_branch' do
+    let(:project) { create(:project) }
+    let(:environment) { create(:environment, project: project) }
+
+    context 'when last deployment to environment is the most recent one' do
+      before do
+        create(:deployment, environment: environment, ref: 'feature')
+      end
+
+      it 'finds recently updated environment' do
+        expect(project.environments_recently_updated_on_branch('feature'))
+          .to contain_exactly(environment)
+      end
+    end
+
+    context 'when last deployment to environment is not the most recent' do
+      before do
+        create(:deployment, environment: environment, ref: 'feature')
+        create(:deployment, environment: environment, ref: 'master')
+      end
+
+      it 'does not find environment' do
+        expect(project.environments_recently_updated_on_branch('feature'))
+          .to be_empty
+      end
+    end
+
+    context 'when there are two environments that deploy to the same branch' do
+      let(:second_environment) { create(:environment, project: project) }
+
+      before do
+        create(:deployment, environment: environment, ref: 'feature')
+        create(:deployment, environment: second_environment, ref: 'feature')
+      end
+
+      it 'finds both environments' do
+        expect(project.environments_recently_updated_on_branch('feature'))
+          .to contain_exactly(environment, second_environment)
       end
     end
   end

spec/services/after_branch_delete_service_spec.rb

+require 'spec_helper'
+
+describe AfterBranchDeleteService, services: true do
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let(:service) { described_class.new(project, user) }
+
+  describe '#execute' do
+    it 'stops environments attached to branch' do
+      expect(service).to receive(:stop_environments)
+
+      service.execute('feature')
+    end
+  end
+end

spec/services/ci/stop_environments_service_spec.rb

+require 'spec_helper'
+
+describe Ci::StopEnvironmentsService, services: true do
+  let(:project) { create(:project, :private) }
+  let(:user) { create(:user) }
+
+  let(:service) { described_class.new(project, user) }
+
+  describe '#execute' do
+    context 'when environment with review app exists' do
+      before do
+        create(:environment, :with_review_app, project: project,
+                                               ref: 'feature')
+      end
+
+      context 'when user has permission to stop environment' do
+        before do
+          project.team << [user, :developer]
+        end
+
+        context 'when environment is associated with removed branch' do
+          it 'stops environment' do
+            expect_environment_stopped_on('feature')
+          end
+        end
+
+        context 'when environment is associated with different branch' do
+          it 'does not stop environment' do
+            expect_environment_not_stopped_on('master')
+          end
+        end
+
+        context 'when specified branch does not exist' do
+          it 'does not stop environment' do
+            expect_environment_not_stopped_on('non/existent/branch')
+          end
+        end
+
+        context 'when no branch not specified' do
+          it 'does not stop environment' do
+            expect_environment_not_stopped_on(nil)
+          end
+        end
+
+        context 'when environment is not stoppable' do
+          before do
+            allow_any_instance_of(Environment)
+              .to receive(:stoppable?).and_return(false)
+          end
+
+          it 'does not stop environment' do
+            expect_environment_not_stopped_on('feature')
+          end
+        end
+      end
+
+      context 'when user does not have permission to stop environment' do
+        before do
+          project.team << [user, :guest]
+        end
+
+        it 'does not stop environment' do
+          expect_environment_not_stopped_on('master')
+        end
+      end
+    end
+
+    context 'when there is no environment associated with review app' do
+      before do
+        create(:environment, project: project)
+      end
+
+      context 'when user has permission to stop environments' do
+        before do
+          project.team << [user, :master]
+        end
+
+        it 'does not stop environment' do
+          expect_environment_not_stopped_on('master')
+        end
+      end
+    end
+
+    context 'when environment does not exist' do
+      it 'does not raise error' do
+        expect { service.execute('master') }
+          .not_to raise_error
+      end
+    end
+  end
+
+  def expect_environment_stopped_on(branch)
+    expect_any_instance_of(Environment)
+      .to receive(:stop!)
+
+    service.execute(branch)
+  end
+
+  def expect_environment_not_stopped_on(branch)
+    expect_any_instance_of(Environment)
+      .not_to receive(:stop!)
+
+    service.execute(branch)
+  end
+end

spec/services/delete_branch_service_spec.rb

+require 'spec_helper'
+
+describe DeleteBranchService, services: true do
+  let(:project) { create(:project) }
+  let(:repository) { project.repository }
+  let(:user) { create(:user) }
+  let(:service) { described_class.new(project, user) }
+
+  describe '#execute' do
+    context 'when user has access to push to repository' do
+      before do
+        project.team << [user, :developer]
+      end
+
+      it 'removes the branch' do
+        expect(branch_exists?('feature')).to be true
+
+        result = service.execute('feature')
+
+        expect(result[:status]).to eq :success
+        expect(branch_exists?('feature')).to be false
+      end
+    end
+
+    context 'when user does not have access to push to repository' do
+      it 'does not remove branch' do
+        expect(branch_exists?('feature')).to be true
+
+        result = service.execute('feature')
+
+        expect(result[:status]).to eq :error
+        expect(result[:message]).to eq 'You dont have push access to repo'
+        expect(branch_exists?('feature')).to be true
+      end
+    end
+  end
+
+  def branch_exists?(branch_name)
+    repository.ref_exists?("refs/heads/#{branch_name}")
+  end
+end