Commit 5d3fc8bc authored by Shinya Maeda's avatar Shinya Maeda
Browse files

Merge branch '332301-environments-should-not-show-as-stopped-if-on_stop-fails' into 'master'

Resolve "Environments should not show as stopped if on_stop fails"

See merge request gitlab-org/gitlab!86478
parents 6ce9b8fa ace87924
......@@ -568,6 +568,10 @@ def on_stop
options&.dig(:environment, :on_stop)
end
 
def stop_action_successful?
Feature.disabled?(:env_stopped_on_stop_success, project) || success?
end
##
# All variables, including persisted environment variables.
#
......
......@@ -132,10 +132,16 @@ class Environment < ApplicationRecord
end
 
event :stop do
transition available: :stopped
transition available: :stopping, if: :wait_for_stop?
transition available: :stopped, unless: :wait_for_stop?
end
event :stop_complete do
transition %i(available stopping) => :stopped
end
 
state :available
state :stopping
state :stopped
 
before_transition any => :stopped do |environment|
......@@ -293,6 +299,10 @@ def cancel_deployment_jobs!
end
end
 
def wait_for_stop?
stop_actions.present? && Feature.enabled?(:env_stopped_on_stop_success, project)
end
def stop_with_actions!(current_user)
return unless available?
 
......
......@@ -7,7 +7,11 @@ class StopService < BaseService
def execute(environment)
return unless can?(current_user, :stop_environment, environment)
 
environment.stop_with_actions!(current_user)
if params[:force]
environment.stop_complete!
else
environment.stop_with_actions!(current_user)
end
end
 
def execute_for_branch(branch_name)
......
......@@ -13,13 +13,13 @@ class BuildSuccessWorker # rubocop:disable Scalability/IdempotentWorker
 
def perform(build_id)
Ci::Build.find_by_id(build_id).try do |build|
stop_environment(build) if build.stops_environment?
stop_environment(build) if build.stops_environment? && build.stop_action_successful?
end
end
 
private
 
def stop_environment(build)
build.persisted_environment.fire_state_event(:stop)
build.persisted_environment.fire_state_event(:stop_complete)
end
end
---
name: env_stopped_on_stop_success
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/86478
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/361473
milestone: '15.0'
type: development
group: group::release
default_enabled: false
......@@ -15,12 +15,12 @@ Get all environments for a given project.
GET /projects/:id/environments
```
 
| Attribute | Type | Required | Description |
| --------- | ------- | -------- | --------------------- |
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](index.md#namespaced-path-encoding) owned by the authenticated user |
| `name` | string | no | Return the environment with this name. Mutually exclusive with `search` |
| `search` | string | no | Return list of environments matching the search criteria. Mutually exclusive with `name` |
| `states` | string | no | List all environments that match a specific state. Accepted values: `available` or `stopped`. If no state value given, returns all environments. |
| Attribute | Type | Required | Description |
| --------- | ------- | -------- |-------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](index.md#namespaced-path-encoding) owned by the authenticated user |
| `name` | string | no | Return the environment with this name. Mutually exclusive with `search` |
| `search` | string | no | Return list of environments matching the search criteria. Mutually exclusive with `name` |
| `states` | string | no | List all environments that match a specific state. Accepted values: `available`, `stopping` or `stopped`. If no state value given, returns all environments. |
 
```shell
curl --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/environments?name=review%2Ffix-foo"
......@@ -382,10 +382,11 @@ It returns `200` if the environment was successfully stopped, and `404` if the e
POST /projects/:id/environments/:environment_id/stop
```
 
| Attribute | Type | Required | Description |
| --------- | ------- | -------- | --------------------- |
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](index.md#namespaced-path-encoding) owned by the authenticated user |
| `environment_id` | integer | yes | The ID of the environment |
| Attribute | Type | Required | Description |
|------------------|----------------|----------|----------------------------------------------------------------------------------------------------------------|
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](index.md#namespaced-path-encoding) owned by the authenticated user |
| `environment_id` | integer | yes | The ID of the environment |
| `force` | boolean | no | Force environment to stop even when `on_stop` action fails |
 
```shell
curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/environments/1/stop"
......
......@@ -24,7 +24,7 @@
subject { user.can?(:destroy_environment, environment) }
 
before do
environment.stop!
environment.stop_complete!
end
 
it_behaves_like 'protected environments access'
......
......@@ -128,14 +128,14 @@ class Environments < ::API::Base
end
params do
requires :environment_id, type: Integer, desc: 'The environment ID'
optional :force, type: Boolean, default: false
end
post ':id/environments/:environment_id/stop' do
authorize! :read_environment, user_project
 
environment = user_project.environments.find(params[:environment_id])
authorize! :stop_environment, environment
environment.stop_with_actions!(current_user)
::Environments::StopService.new(user_project, current_user, declared_params(include_missing: false))
.execute(environment)
 
status 200
present environment, with: Entities::Environment, current_user: current_user
......
......@@ -284,7 +284,6 @@ def auto_stop_button_selector
click_button('Stop')
click_button('Stop environment') # confirm modal
wait_for_all_requests
expect(page).to have_button('Delete')
end
end
 
......@@ -362,8 +361,6 @@ def auto_stop_button_selector
end
 
visit_environment(environment)
expect(page).not_to have_button('Stop')
end
 
##
......
......@@ -586,6 +586,24 @@
expect(action).to eq(close_action)
expect(action.user).to eq(user)
end
context 'env_stopped_on_stop_success feature flag' do
it 'environment is not stopped when flag is enabled' do
stub_feature_flags(env_stopped_on_stop_success: true)
subject
expect(environment).not_to be_stopped
end
it 'environment is stopped when flag is disabled' do
stub_feature_flags(env_stopped_on_stop_success: false)
subject
expect(environment).to be_stopped
end
end
end
 
context 'if action did finish' do
......@@ -1730,17 +1748,17 @@
let!(:environment3) { create(:environment, project: project, state: 'stopped') }
 
it 'returns the environments count grouped by state' do
expect(project.environments.count_by_state).to eq({ stopped: 2, available: 1 })
expect(project.environments.count_by_state).to eq({ stopped: 2, available: 1, stopping: 0 })
end
 
it 'returns the environments count grouped by state with zero value' do
environment2.update!(state: 'stopped')
expect(project.environments.count_by_state).to eq({ stopped: 3, available: 0 })
expect(project.environments.count_by_state).to eq({ stopped: 3, available: 0, stopping: 0 })
end
end
 
it 'returns zero state counts when environments are empty' do
expect(project.environments.count_by_state).to eq({ stopped: 0, available: 0 })
expect(project.environments.count_by_state).to eq({ stopped: 0, available: 0, stopping: 0 })
end
end
 
......
......@@ -84,7 +84,7 @@
 
context 'and environment is stopped' do
before do
environment.stop
environment.stop_complete
end
 
it 'makes environment available' do
......
......@@ -37,7 +37,7 @@
it 'stops environments and play stop jobs' do
expect { subject }
.to change { Environment.all.map(&:state).uniq }
.from(['available']).to(['stopped'])
.from(['available']).to(['stopping'])
 
expect(Ci::Build.where(name: 'stop_review_app').map(&:status).uniq).to eq(['pending'])
end
......
......@@ -29,14 +29,27 @@
review_job.success!
end
 
it 'stops the environment' do
expect { subject }.to change { environment.reload.state }.from('available').to('stopped')
context 'without stop action' do
let!(:environment) { create(:environment, :available, project: project) }
it 'stops the environment' do
expect { subject }.to change { environment.reload.state }.from('available').to('stopped')
end
end
 
it 'plays the stop action' do
expect { subject }.to change { stop_review_job.reload.status }.from('manual').to('pending')
end
 
context 'force option' do
let(:service) { described_class.new(project, user, { force: true }) }
it 'does not play the stop action when forced' do
expect { subject }.to change { environment.reload.state }.from('available').to('stopped')
expect(stop_review_job.reload.status).to eq('manual')
end
end
context 'when an environment has already been stopped' do
let!(:environment) { create(:environment, :stopped, project: project) }
 
......@@ -77,7 +90,7 @@
 
context 'when environment is associated with removed branch' do
it 'stops environment' do
expect_environment_stopped_on('feature')
expect_environment_stopping_on('feature')
end
end
 
......@@ -195,8 +208,7 @@
 
it 'stops the active environment' do
subject
expect(pipeline.environments_in_self_and_descendants.first).to be_stopped
expect(pipeline.environments_in_self_and_descendants.first).to be_stopping
end
 
context 'when pipeline is a branch pipeline for merge request' do
......@@ -268,6 +280,11 @@ def expect_environment_stopped_on(branch)
.to change { Environment.last.state }.from('available').to('stopped')
end
 
def expect_environment_stopping_on(branch)
expect { service.execute_for_branch(branch) }
.to change { Environment.last.state }.from('available').to('stopping')
end
def expect_environment_not_stopped_on(branch)
expect { service.execute_for_branch(branch) }
.not_to change { Environment.last.state }
......
......@@ -8,7 +8,7 @@
 
context 'when build exists' do
context 'when the build will stop an environment' do
let!(:build) { create(:ci_build, :stop_review_app, environment: environment.name, project: environment.project) }
let!(:build) { create(:ci_build, :stop_review_app, environment: environment.name, project: environment.project, status: :success) }
let(:environment) { create(:environment, state: :available) }
 
it 'stops the environment' do
......@@ -18,6 +18,33 @@
 
expect(environment.reload).to be_stopped
end
context 'when the build fails' do
before do
build.update!(status: :failed)
environment.update!(state: :available)
end
it 'does not stop the environment' do
expect(environment).to be_available
stub_feature_flags(env_stopped_on_stop_success: true)
subject
expect(environment.reload).not_to be_stopped
end
it 'does stop the environment when feature flag is disabled' do
expect(environment).to be_available
stub_feature_flags(env_stopped_on_stop_success: false)
subject
expect(environment.reload).to be_stopped
end
end
end
end
 
......
......@@ -23,7 +23,7 @@
it 'stops the environment' do
expect { subject }
.to change { Environment.find_by_name('review/feature').state }
.from('available').to('stopped')
.from('available').to('stopping')
end
 
it 'executes the stop action' do
......
Supports Markdown
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