From 20c04cd48e4ae804330e887eec19fa75e405651e Mon Sep 17 00:00:00 2001 From: Mayra Cabrera Date: Wed, 20 Aug 2025 16:35:03 -0600 Subject: [PATCH 1/2] Reverts 195507 to address production incident Related to https://gitlab.com/gitlab-com/gl-infra/production/-/issues/20397 --- app/services/merge_requests/close_service.rb | 12 +++-- app/services/projects/update_service.rb | 6 +++ .../beta/destroy_fork_network_on_archive.yml | 9 ++++ .../merge_requests/close_service_spec.rb | 23 ++++++++ spec/services/projects/update_service_spec.rb | 54 +++++++++++++++++++ 5 files changed, 99 insertions(+), 5 deletions(-) create mode 100644 config/feature_flags/beta/destroy_fork_network_on_archive.yml diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index 9c1056f3585b58..b9d2098eba395b 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -5,7 +5,13 @@ class CloseService < MergeRequests::BaseService include RemovesRefs def execute(merge_request, commit = nil) - return merge_request unless authorized_to_close?(merge_request) + if Feature.enabled?(:destroy_fork_network_on_archive, project) + unless params[:skip_authorization].present? || can?(current_user, :update_merge_request, merge_request) + return merge_request + end + else + return merge_request unless can?(current_user, :update_merge_request, merge_request) + end # If we close MergeRequest we want to ignore validation # so we can close broken one (Ex. fork project removed) @@ -49,10 +55,6 @@ def expire_unapproved_key(merge_request) def trigger_merge_request_merge_status_updated(merge_request) GraphqlTriggers.merge_request_merge_status_updated(merge_request) end - - def authorized_to_close?(merge_request) - params[:skip_authorization].present? || can?(current_user, :update_merge_request, merge_request) - end end end diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 556b8255b6e5d7..bc740fcc7ff81f 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -37,6 +37,12 @@ def execute update_project! after_update + if Feature.enabled?(:destroy_fork_network_on_archive, project) && + project.previous_changes[:archived] == [false, true] + + UnlinkForkService.new(project, current_user).execute + end + success rescue ActiveRecord::ActiveRecordError update_failed! diff --git a/config/feature_flags/beta/destroy_fork_network_on_archive.yml b/config/feature_flags/beta/destroy_fork_network_on_archive.yml new file mode 100644 index 00000000000000..d6efb391241e0d --- /dev/null +++ b/config/feature_flags/beta/destroy_fork_network_on_archive.yml @@ -0,0 +1,9 @@ +--- +name: destroy_fork_network_on_archive +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/518708 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/189438 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/538925 +milestone: '18.0' +type: beta +group: group::source code +default_enabled: false diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 0bddbc0bbfbdf6..55c9b3ff1d0b90 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -171,5 +171,28 @@ def execute end end end + + context 'with feature flag :destroy_fork_network_on_archive disabled' do + before do + stub_feature_flags(destroy_fork_network_on_archive: false) + end + + context 'when user has permission' do + it 'closes MR' do + result = service.execute(merge_request) + + expect(result).to be_closed + end + end + + context 'when user does not have permission' do + it 'does not close MR' do + guest_service = described_class.new(project: project, current_user: guest) + result = guest_service.execute(merge_request) + + expect(result).to be_open + end + end + end end end diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 48adcf56072ef2..2a984e9a6e8320 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -420,6 +420,60 @@ end end + context 'when archiving a project' do + before do + allow(Projects::UnlinkForkService).to receive(:new).and_return(unlink_fork_service) + end + + let(:unlink_fork_service) { instance_double(Projects::UnlinkForkService, execute: true) } + + it_behaves_like 'publishing Projects::ProjectAttributesChangedEvent', + params: { archived: true }, + attributes: %w[updated_at archived] + + it 'publishes a ProjectTransferedEvent' do + expect { update_project(project, user, archived: true) } + .to publish_event(Projects::ProjectArchivedEvent) + .with( + project_id: project.id, + namespace_id: project.namespace_id, + root_namespace_id: project.root_namespace.id + ) + end + + context 'when the destroy_fork_network_on_archive feature flag is enabled' do + context 'when project is being archived' do + it 'calls UnlinkForkService' do + project.update!(archived: false) + + expect(Projects::UnlinkForkService).to receive(:new).with(project, user).and_return(unlink_fork_service) + + update_project(project, user, archived: true) + end + end + + context 'when project is not being archived' do + it 'does not call UnlinkForkService' do + expect(Projects::UnlinkForkService).not_to receive(:new) + + update_project(project, user, archived: false) + end + end + end + + context 'when the destroy_fork_network_on_archive feature flag is disabled' do + before do + stub_feature_flags(destroy_fork_network_on_archive: false) + end + + it 'does not call UnlinkForkService' do + expect(Projects::UnlinkForkService).not_to receive(:new) + + update_project(project, user, archived: true) + end + end + end + context 'when changing operations feature visibility' do let(:feature_params) { { operations_access_level: ProjectFeature::DISABLED } } -- GitLab From 4952be39c808e2d5a45007d0c34b043000a9f073 Mon Sep 17 00:00:00 2001 From: Fred Reinink Date: Wed, 20 Aug 2025 17:12:25 -0600 Subject: [PATCH 2/2] Move feature flag check to archive service --- app/services/projects/archive_service.rb | 2 + app/services/projects/update_service.rb | 6 --- .../services/projects/archive_service_spec.rb | 12 +++++ spec/services/projects/update_service_spec.rb | 54 ------------------- 4 files changed, 14 insertions(+), 60 deletions(-) diff --git a/app/services/projects/archive_service.rb b/app/services/projects/archive_service.rb index f53fe6b50bd79f..c8f5e4f06eac22 100644 --- a/app/services/projects/archive_service.rb +++ b/app/services/projects/archive_service.rb @@ -35,6 +35,8 @@ def after_archive system_hook_service.execute_hooks_for(project, :update) publish_events + return unless Feature.enabled?(:destroy_fork_network_on_archive, project) + UnlinkForkService.new(project, current_user).execute end end diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index bc740fcc7ff81f..556b8255b6e5d7 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -37,12 +37,6 @@ def execute update_project! after_update - if Feature.enabled?(:destroy_fork_network_on_archive, project) && - project.previous_changes[:archived] == [false, true] - - UnlinkForkService.new(project, current_user).execute - end - success rescue ActiveRecord::ActiveRecordError update_failed! diff --git a/spec/services/projects/archive_service_spec.rb b/spec/services/projects/archive_service_spec.rb index 584e2142a48efe..0b634a437f70cd 100644 --- a/spec/services/projects/archive_service_spec.rb +++ b/spec/services/projects/archive_service_spec.rb @@ -101,6 +101,18 @@ service.execute end + context 'with feature flag destroy_fork_network_on_archive disabled' do + before do + stub_feature_flags(destroy_fork_network_on_archive: false) + end + + it 'does not unlink fork' do + expect(unlink_fork_service).not_to receive(:execute) + + service.execute + end + end + it 'publishes a ProjectArchivedEvent' do expect { service.execute }.to publish_event(Projects::ProjectArchivedEvent) .with( diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 2a984e9a6e8320..48adcf56072ef2 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -420,60 +420,6 @@ end end - context 'when archiving a project' do - before do - allow(Projects::UnlinkForkService).to receive(:new).and_return(unlink_fork_service) - end - - let(:unlink_fork_service) { instance_double(Projects::UnlinkForkService, execute: true) } - - it_behaves_like 'publishing Projects::ProjectAttributesChangedEvent', - params: { archived: true }, - attributes: %w[updated_at archived] - - it 'publishes a ProjectTransferedEvent' do - expect { update_project(project, user, archived: true) } - .to publish_event(Projects::ProjectArchivedEvent) - .with( - project_id: project.id, - namespace_id: project.namespace_id, - root_namespace_id: project.root_namespace.id - ) - end - - context 'when the destroy_fork_network_on_archive feature flag is enabled' do - context 'when project is being archived' do - it 'calls UnlinkForkService' do - project.update!(archived: false) - - expect(Projects::UnlinkForkService).to receive(:new).with(project, user).and_return(unlink_fork_service) - - update_project(project, user, archived: true) - end - end - - context 'when project is not being archived' do - it 'does not call UnlinkForkService' do - expect(Projects::UnlinkForkService).not_to receive(:new) - - update_project(project, user, archived: false) - end - end - end - - context 'when the destroy_fork_network_on_archive feature flag is disabled' do - before do - stub_feature_flags(destroy_fork_network_on_archive: false) - end - - it 'does not call UnlinkForkService' do - expect(Projects::UnlinkForkService).not_to receive(:new) - - update_project(project, user, archived: true) - end - end - end - context 'when changing operations feature visibility' do let(:feature_params) { { operations_access_level: ProjectFeature::DISABLED } } -- GitLab