diff --git a/ee/app/models/ee/deployment.rb b/ee/app/models/ee/deployment.rb index ba55e02f573a13c74633fc726956bb37ceb2c43b..9d1e370d66868194d9d0ffc6908344a20826e8b3 100644 --- a/ee/app/models/ee/deployment.rb +++ b/ee/app/models/ee/deployment.rb @@ -31,13 +31,6 @@ module Deployment end end - override :sync_status_with - def sync_status_with(build) - return update_status!('blocked') if build.status == 'manual' && needs_approval? - - super - end - def pending_approval_count environment.required_approval_count - approvals.approved.count end diff --git a/ee/app/services/deployments/approval_service.rb b/ee/app/services/deployments/approval_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..8c19b2d42afda43d21369097f26bcb9d857515f8 --- /dev/null +++ b/ee/app/services/deployments/approval_service.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module Deployments + class ApprovalService < ::BaseService + def execute(deployment, status) + error_message = validate(deployment, status) + return error(error_message) if error_message + + approval = upsert_approval(deployment, status) + return error(approval.errors.full_messages) if approval.errors.any? + + process_build!(deployment, approval) + + success(approval: approval) + end + + private + + def upsert_approval(deployment, status) + if (approval = deployment.approvals.find_by_user_id(current_user.id)) + return approval if approval.status == status + + approval.tap { |a| a.update(status: status) } + else + deployment.approvals.create(user: current_user, status: status) + end + end + + def process_build!(deployment, approval) + return unless deployment.deployable + + if approval.rejected? + deployment.deployable.drop!(:deployment_rejected) + elsif deployment.pending_approval_count <= 0 + deployment.deployable.enqueue! + end + end + + def validate(deployment, status) + return 'Unrecognized status' unless Deployments::Approval.statuses.include?(status) + + return 'This environment is not protected' unless deployment.environment.protected? + + return 'You do not have permission to approve or reject this deployment' unless current_user&.can?(:update_deployment, deployment) + + return 'This deployment job is not waiting for approvals' unless deployment.blocked? + + 'The same user can not approve' if deployment.user == current_user && status == 'approved' + end + end +end diff --git a/ee/app/services/ee/ci/process_build_service.rb b/ee/app/services/ee/ci/process_build_service.rb index 02752b065a4d12a2bf96dcd4db3026485b8c6301..99c9a7646fd23ca70912f39b73e69ccb56d3889c 100644 --- a/ee/app/services/ee/ci/process_build_service.rb +++ b/ee/app/services/ee/ci/process_build_service.rb @@ -6,8 +6,12 @@ module ProcessBuildService override :enqueue def enqueue(build) + # TODO: Refactor to check these conditions before actionizing or scheduling a build. See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/75710#note_756445822 if build.persisted_environment.try(:protected_from?, build.user) return build.drop!(:protected_environment_failure) + elsif build.persisted_environment.try(:needs_approval?) + build.actionize! + return build.deployment&.block! end super diff --git a/ee/spec/factories/deployments/approval.rb b/ee/spec/factories/deployments/approval.rb index 64be3cafefcadcf084f4b92d298f55186a0431bd..f62e8610221e3dfae98131befd1faa1701185368 100644 --- a/ee/spec/factories/deployments/approval.rb +++ b/ee/spec/factories/deployments/approval.rb @@ -4,10 +4,10 @@ factory :deployment_approval, class: 'Deployments::Approval' do user deployment - status { :approved } + status { 'approved' } trait :rejected do - status { :rejected } + status { 'rejected' } end end end diff --git a/ee/spec/models/deployment_spec.rb b/ee/spec/models/deployment_spec.rb index 0be6b756d7355de15e607b1ea5fa5f9cc2006353..3b08f562ff12bfaf2f6a5d234dd3ecaf0677dda9 100644 --- a/ee/spec/models/deployment_spec.rb +++ b/ee/spec/models/deployment_spec.rb @@ -24,67 +24,6 @@ end end - describe '#sync_status_with' do - subject { deployment.sync_status_with(ci_build) } - - let_it_be(:project) { create(:project, :repository) } - - let(:environment) { create(:environment, project: project) } - let(:deployment) { create(:deployment, project: project, environment: environment) } - - context 'when build is manual' do - let(:ci_build) { create(:ci_build, project: project, status: :manual) } - - context 'and Protected Environments feature is available' do - before do - stub_licensed_features(protected_environments: true) - create(:protected_environment, name: environment.name, project: project, required_approval_count: required_approval_count) - end - - context 'and deployment needs approval' do - let(:required_approval_count) { 1 } - - it 'blocks the deployment' do - expect(Gitlab::ErrorTracking).not_to receive(:track_exception) - - is_expected.to eq(true) - - expect(deployment.status).to eq('blocked') - expect(deployment.errors).to be_empty - end - end - - context 'and deployment does not need approval' do - let(:required_approval_count) { 0 } - - it 'does not change the deployment' do - expect(Gitlab::ErrorTracking).not_to receive(:track_exception) - - is_expected.to eq(false) - - expect(deployment.status).to eq('created') - expect(deployment.errors).to be_empty - end - end - end - - context 'and Protected Environments feature is not available' do - before do - stub_licensed_features(protected_environments: false) - end - - it 'does not change the deployment' do - expect(Gitlab::ErrorTracking).not_to receive(:track_exception) - - is_expected.to eq(false) - - expect(deployment.status).to eq('created') - expect(deployment.errors).to be_empty - end - end - end - end - describe '#pending_approval_count' do let_it_be(:project) { create(:project, :repository) } diff --git a/ee/spec/services/ci/process_build_service_spec.rb b/ee/spec/services/ci/process_build_service_spec.rb index 5e3abe0c7123fccae00cbba4836df90f7a2a28f6..0d5b9d301c849e7cd4bbab41155b3b0f3025ad2a 100644 --- a/ee/spec/services/ci/process_build_service_spec.rb +++ b/ee/spec/services/ci/process_build_service_spec.rb @@ -52,6 +52,28 @@ expect(ci_build.pending?).to be_truthy end + + context 'and environment needs approval' do + before do + protected_environment.update!(required_approval_count: 1) + end + + it 'makes the build a manual action' do + expect { subject }.to change { ci_build.status }.from('created').to('manual') + end + + context 'and the build has a deployment' do + let(:deployment) { create(:deployment, deployable: ci_build, environment: environment, user: user, project: project) } + + it 'blocks the deployment' do + expect { subject }.to change { deployment.reload.status }.from('created').to('blocked') + end + + it 'makes the build a manual action' do + expect { subject }.to change { ci_build.status }.from('created').to('manual') + end + end + end end end end diff --git a/ee/spec/services/deployments/approval_service_spec.rb b/ee/spec/services/deployments/approval_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..bc07b9f49fdc9fc3cccc41a2e8e73629c8d113bf --- /dev/null +++ b/ee/spec/services/deployments/approval_service_spec.rb @@ -0,0 +1,181 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Deployments::ApprovalService do + let_it_be(:project) { create(:project, :repository) } + + let(:service) { described_class.new(project, user) } + let(:user) { create(:user) } + let(:environment) { create(:environment, project: project) } + let(:status) { 'approved' } + let(:required_approval_count) { 2 } + let(:build) { create(:ci_build, :manual, project: project) } + let(:deployment) { create(:deployment, :blocked, project: project, environment: environment, deployable: build) } + + before do + stub_licensed_features(protected_environments: true) + create(:protected_environment, :maintainers_can_deploy, name: environment.name, project: project, required_approval_count: required_approval_count) + project.add_maintainer(user) + end + + shared_examples_for 'error' do |message:| + it 'returns an error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq(message) + end + end + + shared_examples_for 'reject' do + it 'rejects the deployment' do + expect(subject[:status]).to eq(:success) + expect(subject[:approval].status).to eq('rejected') + expect(subject[:approval].user).to eq(user) + expect(subject[:approval].deployment).to eq(deployment) + expect(deployment.approvals.approved.count).to eq(0) + expect(deployment.approvals.rejected.count).to eq(1) + end + end + + shared_examples_for 'approve' do + it 'approves the deployment' do + expect(subject[:status]).to eq(:success) + expect(subject[:approval].status).to eq('approved') + expect(subject[:approval].user).to eq(user) + expect(subject[:approval].deployment).to eq(deployment) + expect(deployment.approvals.approved.count).to eq(1) + expect(deployment.approvals.rejected.count).to eq(0) + end + end + + describe '#execute' do + subject { service.execute(deployment, status) } + + context 'when status is approved' do + include_examples 'approve' + end + + context 'when status is rejected' do + let(:status) { 'rejected' } + + include_examples 'reject' + end + + context 'when user already approved' do + before do + service.execute(deployment, :approved) + end + + context 'and is approving again' do + include_examples 'approve' + end + + context 'and is rejecting' do + let(:status) { 'rejected' } + + include_examples 'reject' + end + end + + context 'processing the build' do + context 'when build is nil' do + before do + deployment.deployable = nil + end + + it 'does not raise an error' do + expect { subject }.not_to raise_error + end + end + + context 'when deployment was rejected' do + let(:status) { 'rejected' } + + it 'drops the build' do + subject + + expect(deployment.deployable.status).to eq('failed') + expect(deployment.deployable.failure_reason).to eq('deployment_rejected') + end + end + + context 'when no additional approvals are required' do + let(:required_approval_count) { 1 } + + it 'enqueues the build' do + subject + + expect(deployment.deployable.status).to eq('pending') + end + end + + context 'when additional approvals are required' do + let(:required_approval_count) { 2 } + + it 'does not change the build' do + expect { subject }.not_to change { deployment.deployable.reload.status } + end + end + end + + context 'validations' do + context 'when status is not recognized' do + let(:status) { 'foo' } + + include_examples 'error', message: 'Unrecognized status' + end + + context 'when environment is not protected' do + let(:deployment) { create(:deployment, project: project, deployable: build) } + + include_examples 'error', message: 'This environment is not protected' + end + + context 'when Protected Environments feature is not available' do + before do + stub_licensed_features(protected_environments: false) + end + + include_examples 'error', message: 'This environment is not protected' + end + + context 'when the user does not have permission to update deployment' do + before do + project.add_developer(user) + end + + include_examples 'error', message: 'You do not have permission to approve or reject this deployment' + end + + context 'when user is nil' do + let(:user) { nil } + + include_examples 'error', message: 'You do not have permission to approve or reject this deployment' + end + + context 'when deployment is not blocked' do + let(:deployment) { create(:deployment, project: project, environment: environment, deployable: build) } + + include_examples 'error', message: 'This deployment job is not waiting for approvals' + end + + context 'when the creator of the deployment is approving' do + before do + deployment.user = user + end + + include_examples 'error', message: 'The same user can not approve' + end + + context 'when the creator of the deployment is rejecting' do + let(:status) { 'rejected' } + + before do + deployment.user = user + end + + include_examples 'reject' + end + end + end +end diff --git a/spec/factories/deployments.rb b/spec/factories/deployments.rb index 2aab9764560cc92f54743b42cc375ff4c10340e3..ab1b794632aeb2044a697bcf074bb7780e13c6a9 100644 --- a/spec/factories/deployments.rb +++ b/spec/factories/deployments.rb @@ -55,6 +55,10 @@ status { :created } end + trait :blocked do + status { :blocked } + end + # This trait hooks the state maechine's events trait :succeed do after(:create) do |deployment, evaluator|