diff --git a/db/migrate/20230328150343_add_retried_at_to_status_check_responses.rb b/db/migrate/20230328150343_add_retried_at_to_status_check_responses.rb new file mode 100644 index 0000000000000000000000000000000000000000..53cc1f0432b00aa397aead66d15cbf96082e3837 --- /dev/null +++ b/db/migrate/20230328150343_add_retried_at_to_status_check_responses.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddRetriedAtToStatusCheckResponses < Gitlab::Database::Migration[2.1] + def change + add_column :status_check_responses, :retried_at, :datetime_with_timezone + end +end diff --git a/db/schema_migrations/20230328150343 b/db/schema_migrations/20230328150343 new file mode 100644 index 0000000000000000000000000000000000000000..05054b0490b898a3e0456fb3917e2410bba16abe --- /dev/null +++ b/db/schema_migrations/20230328150343 @@ -0,0 +1 @@ +d5cb88bd614c000b9b782e8a827bf4efcf04c57688bd4bde3d01f555b52f43fb \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 440e5a3e086edd4a49a7880095a9cfee62f4e8c3..0d00dfc005a114f3c9bf412630447fc6d947a935 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -22865,7 +22865,8 @@ CREATE TABLE status_check_responses ( external_approval_rule_id bigint, sha bytea NOT NULL, external_status_check_id bigint NOT NULL, - status smallint DEFAULT 0 NOT NULL + status smallint DEFAULT 0 NOT NULL, + retried_at timestamp with time zone ); CREATE SEQUENCE status_check_responses_id_seq diff --git a/ee/app/models/merge_requests/external_status_check.rb b/ee/app/models/merge_requests/external_status_check.rb index eb53e0290dd9654f572a17c6537a21022ce9bbb6..fd10c700653e993d69ad4b3b4196dce35d660826 100644 --- a/ee/app/models/merge_requests/external_status_check.rb +++ b/ee/app/models/merge_requests/external_status_check.rb @@ -32,11 +32,19 @@ def async_execute(data) end def status(merge_request, sha) - merge_request.status_check_responses.where(external_status_check: self, sha: sha).last&.status || 'pending' + last_response = response_for(merge_request, sha) + + return 'pending' if !last_response || !last_response.retried_at.nil? + + last_response.status end def failed?(merge_request) - merge_request.status_check_responses.where(external_status_check: self, sha: merge_request.diff_head_sha).last&.status == 'failed' + status(merge_request, merge_request.diff_head_sha) == 'failed' + end + + def response_for(merge_request, sha) + merge_request.status_check_responses.order(id: :desc).find_by(external_status_check: self, sha: sha) end def to_h diff --git a/ee/app/services/external_status_checks/retry_service.rb b/ee/app/services/external_status_checks/retry_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..4a227da4238ee643bb02cadde11d2f66bd4093e6 --- /dev/null +++ b/ee/app/services/external_status_checks/retry_service.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module ExternalStatusChecks + class RetryService < BaseService + def execute(rule) + return unauthorized_error_response unless current_user.can?(:retry_failed_status_checks, params[:merge_request]) + return not_failed_error_response unless rule.failed?(params[:merge_request]) + + last_response = rule.response_for(params[:merge_request], params[:merge_request].diff_head_sha) + + if last_response.update(retried_at: Time.current) + data = params[:merge_request].to_hook_data(current_user) + rule.async_execute(data) + ServiceResponse.success(payload: { rule: rule }) + else + ServiceResponse.error(message: 'Failed to retry rule', + payload: { errors: rule.errors.full_messages }, + reason: :unprocessable_entity) + end + end + + private + + def not_failed_error_response + ServiceResponse.error( + message: 'Failed to retry rule', + payload: { errors: 'External status check must be failed' }, + reason: :unprocessable_entity + ) + end + + def unauthorized_error_response + ServiceResponse.error( + message: 'Failed to retry rule', + payload: { errors: 'Not allowed' }, + reason: :unauthorized + ) + end + end +end diff --git a/ee/lib/api/status_checks.rb b/ee/lib/api/status_checks.rb index c382a18f6f671ae9dc2fd9c5d0ad9b1af4770299..8536e3f3b7978eec499365d9eac5babf6e4e6e40 100644 --- a/ee/lib/api/status_checks.rb +++ b/ee/lib/api/status_checks.rb @@ -173,18 +173,20 @@ def check_feature_enabled! requires :external_status_check_id, type: Integer, desc: 'ID of a failed external status check' end post ':external_status_check_id/retry' do - merge_request = find_merge_request_with_access(params[:merge_request_iid], :approve_merge_request) - - not_found! unless current_user.can?(:retry_failed_status_checks, merge_request) - + merge_request = find_merge_request_with_access(params[:merge_request_iid], :retry_failed_status_checks) status_check = merge_request.project.external_status_checks.find(params[:external_status_check_id]) + service = ::ExternalStatusChecks::RetryService.new( + container: user_project, + current_user: current_user, + params: { merge_request: merge_request } + ) + + response = service.execute(status_check) - if status_check.failed?(merge_request) - data = merge_request.to_hook_data(current_user) - status_check.async_execute(data) + if response.success? accepted! else - unprocessable_entity!("External status check must be failed") + render_api_error!(response.payload[:errors], response.reason) end end end diff --git a/ee/spec/models/merge_requests/external_status_check_spec.rb b/ee/spec/models/merge_requests/external_status_check_spec.rb index 82f205807badae376ff50ff4f5ca3d01f42c2b1b..1837512adb3e7955075c0d6e924f1a0956726af8 100644 --- a/ee/spec/models/merge_requests/external_status_check_spec.rb +++ b/ee/spec/models/merge_requests/external_status_check_spec.rb @@ -153,6 +153,30 @@ context 'when a rule has no status check response' do it { is_expected.to eq('pending') } end + + context 'when a rule has already been retried' do + let_it_be(:status_check_response) { create(:status_check_response, merge_request: merge_request, external_status_check: rule, sha: merge_request.source_branch_sha, status: 'failed', retried_at: Time.current) } + + it { is_expected.to eq('pending') } + end + end + + describe 'response_for' do + let_it_be(:rule) { create(:external_status_check) } + let_it_be(:merge_request) { create(:merge_request) } + let_it_be(:first_response) { create(:status_check_response, merge_request: merge_request, external_status_check: rule, sha: merge_request.source_branch_sha) } + let_it_be(:second_response) { create(:status_check_response, merge_request: merge_request, external_status_check: rule, sha: merge_request.source_branch_sha, status: 'failed') } + + let(:project) { merge_request.source_project } + + subject { rule.response_for(merge_request, merge_request.source_branch_sha) } + + before do + first_response + second_response + end + + it { is_expected.to eq(second_response) } end describe 'callbacks', :request_store do diff --git a/ee/spec/models/merge_requests/status_check_response_spec.rb b/ee/spec/models/merge_requests/status_check_response_spec.rb index df8942e4dfa4d4e783701b597cd675185cf77255..a7814969cb508e0d4505fcd956ca9556b4ff433f 100644 --- a/ee/spec/models/merge_requests/status_check_response_spec.rb +++ b/ee/spec/models/merge_requests/status_check_response_spec.rb @@ -8,7 +8,7 @@ it { is_expected.to belong_to(:merge_request) } it { is_expected.to belong_to(:external_status_check).class_name('MergeRequests::ExternalStatusCheck') } - it { is_expected.to define_enum_for(:status) } + it { is_expected.to define_enum_for(:status).with([:passed, :failed]) } it { is_expected.to validate_presence_of(:merge_request) } it { is_expected.to validate_presence_of(:external_status_check) } diff --git a/ee/spec/requests/api/status_checks_spec.rb b/ee/spec/requests/api/status_checks_spec.rb index 4fd80c651b588445ba33719b12477a4563dbc660..5dec3d3da6fcd8ea2af63a39426f5a5e7c64518b 100644 --- a/ee/spec/requests/api/status_checks_spec.rb +++ b/ee/spec/requests/api/status_checks_spec.rb @@ -313,7 +313,7 @@ end end - describe 'POST :id/merge_requests/:merge_request_iid/status_checks/:external_status_check_id' do + describe 'POST projects/:id/merge_requests/:merge_request_iid/status_checks/:external_status_check_id/retry' do subject(:retry_failed_status_check) do post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/status_checks/#{rule.id}/retry", user) end @@ -395,6 +395,13 @@ expect(response).to have_gitlab_http_status(:accepted) end + + it 'updates last status check response' do + retry_failed_status_check + + expect(merge_request.status_check_responses.last.status).to eq('failed') + expect(merge_request.status_check_responses.last.retried_at).not_to be_nil + end end context 'when status check is passed' do diff --git a/ee/spec/services/external_status_checks/retry_service_spec.rb b/ee/spec/services/external_status_checks/retry_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..fec56f382e39d561370a7a768afd68eb64e476bf --- /dev/null +++ b/ee/spec/services/external_status_checks/retry_service_spec.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ExternalStatusChecks::RetryService, feature_category: :groups_and_projects do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:rule) { create(:external_status_check, project: project) } + let_it_be(:merge_request) do + create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project) + end + + let!(:status_check_response) do + create(:status_check_response, + external_status_check: rule, + merge_request: merge_request, + sha: merge_request.diff_head_sha, + status: status) + end + + subject do + described_class.new( + container: project, + current_user: user, + params: { merge_request: merge_request } + ).execute(rule) + end + + context 'when user has permissions' do + let(:user) { project.first_owner } + + context 'when licensed feature `external_status_checks` is available' do + before do + stub_licensed_features(external_status_checks: true) + end + + context 'when status check response status is `failed`' do + let(:status) { 'failed' } + + context 'when rule retry operation suceeds' do + let(:data) { merge_request.to_hook_data(user) } + + it 'updates `retried_at` field for the last status check response and async executes with data' do + expect_any_instance_of(::MergeRequests::ExternalStatusCheck).to receive(:async_execute).with(data) # rubocop:disable RSpec/AnyInstanceOf - It's not the next instance + + subject + + expect(subject.success?).to be true + + status_check_response.reload + + expect(status_check_response.status).to be('failed') + expect(status_check_response.retried_at).not_to be_nil + end + end + + context 'when rule retry operation fails' do + before do + allow_any_instance_of(::MergeRequests::StatusCheckResponse).to receive(:update).and_return(false) # rubocop:disable RSpec/AnyInstanceOf - It's not the next instance + end + + it 'does not update and has an appropriate error' do + subject + + expect(subject.error?).to be true + expect(subject.reason).to be :unprocessable_entity + expect(subject.message).to be('Failed to retry rule') + + status_check_response.reload + + expect(status_check_response.retried_at).to be_nil + end + end + end + + context 'when status check response status is not `failed`' do + let(:status) { 'passed' } + + it 'contains an appropriate response' do + expect(subject.error?).to be true + expect(subject.reason).to eq(:unprocessable_entity) + expect(subject.message).to eq('Failed to retry rule') + expect(subject.payload[:errors]).to eq('External status check must be failed') + end + end + end + end + + context 'when user does not have permissions' do + let_it_be(:user) { create(:user) } + let(:status) { 'failed' } + + before do + project.add_guest(user) + end + + it 'returns an unauthorized response' do + expect(subject.reason).to eq(:unauthorized) + expect(subject.message).to eq('Failed to retry rule') + expect(subject.payload[:errors]).to eq('Not allowed') + end + end +end