Commit 2b3e61fa authored by Kamil Trzciński's avatar Kamil Trzciński Committed by Alejandro Rodríguez

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

Stop environment when branch is deleted

## What does this MR do?

This MR adds a environment teardown service, that is called when user deletes a branch. This most often happens when merge requests is merged.

## Does this MR meet the acceptance criteria?

- [x] [Changelog entry](https://docs.gitlab.com/ce/development/changelog.html) added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing

## What are the relevant issue numbers?

Closes #23218

See merge request !7355
parent 9d742192
...@@ -37,6 +37,10 @@ class Environment < ActiveRecord::Base ...@@ -37,6 +37,10 @@ class Environment < ActiveRecord::Base
state :stopped state :stopped
end end
def recently_updated_on_branch?(ref)
ref.to_s == last_deployment.try(:ref)
end
def last_deployment def last_deployment
deployments.last deployments.last
end end
...@@ -92,6 +96,7 @@ class Environment < ActiveRecord::Base ...@@ -92,6 +96,7 @@ class Environment < ActiveRecord::Base
def stop!(current_user) def stop!(current_user)
return unless stoppable? return unless stoppable?
stop
stop_action.play(current_user) stop_action.play(current_user)
end end
end end
...@@ -692,12 +692,15 @@ class MergeRequest < ActiveRecord::Base ...@@ -692,12 +692,15 @@ class MergeRequest < ActiveRecord::Base
def environments def environments
return [] unless diff_head_commit return [] unless diff_head_commit
@environments ||= @environments ||= begin
begin target_envs = target_project.environments_for(
envs = target_project.environments_for(target_branch, diff_head_commit, with_tags: true) target_branch, commit: diff_head_commit, with_tags: true)
envs.concat(source_project.environments_for(source_branch, diff_head_commit)) if source_project
envs.uniq source_envs = source_project.environments_for(
end source_branch, commit: diff_head_commit) if source_project
(target_envs.to_a + source_envs.to_a).uniq
end
end end
def state_human_name def state_human_name
......
...@@ -1318,22 +1318,30 @@ class Project < ActiveRecord::Base ...@@ -1318,22 +1318,30 @@ class Project < ActiveRecord::Base
Gitlab::Redis.with { |redis| redis.del(pushes_since_gc_redis_key) } Gitlab::Redis.with { |redis| redis.del(pushes_since_gc_redis_key) }
end end
def environments_for(ref, commit, with_tags: false) def environments_for(ref, commit: nil, with_tags: false)
environment_ids = deployments.group(:environment_id). deployments_query = with_tags ? 'ref = ? OR tag IS TRUE' : 'ref = ?'
select(:environment_id)
environment_ids = environment_ids = deployments
if with_tags .where(deployments_query, ref.to_s)
environment_ids.where('ref=? OR tag IS TRUE', ref) .group(:environment_id)
else .select(:environment_id)
environment_ids.where(ref: ref)
end 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) environment.includes_commit?(commit)
end end
end end
def environments_recently_updated_on_branch(branch)
environments_for(branch).select do |environment|
environment.recently_updated_on_branch?(branch)
end
end
private private
def pushes_since_gc_redis_key def pushes_since_gc_redis_key
......
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
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
...@@ -49,10 +49,7 @@ class GitPushService < BaseService ...@@ -49,10 +49,7 @@ class GitPushService < BaseService
update_gitattributes if is_default_branch? update_gitattributes if is_default_branch?
end end
# Update merge requests that may be affected by this push. A new branch execute_related_hooks
# could cause the last commit of a merge request to change.
update_merge_requests
perform_housekeeping perform_housekeeping
end end
...@@ -62,14 +59,24 @@ class GitPushService < BaseService ...@@ -62,14 +59,24 @@ class GitPushService < BaseService
protected protected
def update_merge_requests def execute_related_hooks
UpdateMergeRequestsWorker.perform_async(@project.id, current_user.id, params[:oldrev], params[:newrev], params[:ref]) # 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) EventCreateService.new.push(@project, current_user, build_push_data)
@project.execute_hooks(build_push_data.dup, :push_hooks) @project.execute_hooks(build_push_data.dup, :push_hooks)
@project.execute_services(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 Ci::CreatePipelineService.new(@project, current_user, build_push_data).execute
ProjectCacheWorker.perform_async(@project.id) ProjectCacheWorker.perform_async(@project.id)
if push_remove_branch?
AfterBranchDeleteService
.new(project, current_user)
.execute(branch_name)
end
end end
def perform_housekeeping def perform_housekeeping
......
---
title: Auto-close environment when branch is deleted
merge_request: 7355
author:
...@@ -3,8 +3,9 @@ FactoryGirl.define do ...@@ -3,8 +3,9 @@ FactoryGirl.define do
sha '97de212e80737a608d939f648d959671fb0a0142' sha '97de212e80737a608d939f648d959671fb0a0142'
ref 'master' ref 'master'
tag false tag false
user
project nil project nil
deployable factory: :ci_build
environment factory: :environment environment factory: :environment
after(:build) do |deployment, evaluator| after(:build) do |deployment, evaluator|
......
...@@ -4,5 +4,33 @@ FactoryGirl.define do ...@@ -4,5 +4,33 @@ FactoryGirl.define do
project factory: :empty_project project factory: :empty_project
sequence(:external_url) { |n| "https://env#{n}.example.gitlab.com" } 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
end end
...@@ -6,8 +6,8 @@ feature 'Environments', feature: true do ...@@ -6,8 +6,8 @@ feature 'Environments', feature: true do
given(:role) { :developer } given(:role) { :developer }
background do background do
login_as(user)
project.team << [user, role] project.team << [user, role]
login_as(user)
end end
describe 'when showing environments' do describe 'when showing environments' do
...@@ -16,7 +16,7 @@ feature 'Environments', feature: true do ...@@ -16,7 +16,7 @@ feature 'Environments', feature: true do
given!(:manual) { } given!(:manual) { }
before do before do
visit namespace_project_environments_path(project.namespace, project) visit_environments(project)
end end
context 'shows two tabs' do context 'shows two tabs' do
...@@ -142,7 +142,7 @@ feature 'Environments', feature: true do ...@@ -142,7 +142,7 @@ feature 'Environments', feature: true do
given!(:manual) { } given!(:manual) { }
before do before do
visit namespace_project_environment_path(project.namespace, project, environment) visit_environment(environment)
end end
context 'without deployments' do context 'without deployments' do
...@@ -152,7 +152,9 @@ feature 'Environments', feature: true do ...@@ -152,7 +152,9 @@ feature 'Environments', feature: true do
end end
context 'with deployments' do 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 scenario 'does show deployment SHA' do
expect(page).to have_link(deployment.short_sha) expect(page).to have_link(deployment.short_sha)
...@@ -232,7 +234,7 @@ feature 'Environments', feature: true do ...@@ -232,7 +234,7 @@ feature 'Environments', feature: true do
describe 'when creating a new environment' do describe 'when creating a new environment' do
before do before do
visit namespace_project_environments_path(project.namespace, project) visit_environments(project)
end end
context 'when logged as developer' do context 'when logged as developer' do
...@@ -271,4 +273,56 @@ feature 'Environments', feature: true do ...@@ -271,4 +273,56 @@ feature 'Environments', feature: true do
end end
end 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 end
...@@ -166,4 +166,25 @@ describe Environment, models: true do ...@@ -166,4 +166,25 @@ describe Environment, models: true do
end end
end 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 end
...@@ -1642,15 +1642,18 @@ describe Project, models: true do ...@@ -1642,15 +1642,18 @@ describe Project, models: true do
end end
it 'returns environment when with_tags is set' do 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 end
it 'does not return environment when no with_tags is set' do 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 end
it 'does not return environment when commit is not part of deployment' do 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
end end
...@@ -1660,15 +1663,65 @@ describe Project, models: true do ...@@ -1660,15 +1663,65 @@ describe Project, models: true do
end end
it 'returns environment when ref is set' do 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 end
it 'does not environment when ref is different' do 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 end
it 'does not return environment when commit is not part of deployment' do 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 end
end end
......
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
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
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment