Merge branch 'sh-fix-project-destroy-in-namespace' into 'master'
Defer project destroys within a namespace in Groups::DestroyService#async_execute See merge request !12435
This commit is contained in:
commit
2497254463
5 changed files with 54 additions and 22 deletions
|
@ -219,6 +219,12 @@ class Namespace < ActiveRecord::Base
|
||||||
parent.present?
|
parent.present?
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def soft_delete_without_removing_associations
|
||||||
|
# We can't use paranoia's `#destroy` since this will hard-delete projects.
|
||||||
|
# Project uses `pending_delete` instead of the acts_as_paranoia gem.
|
||||||
|
self.deleted_at = Time.now
|
||||||
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
def repository_storage_paths
|
def repository_storage_paths
|
||||||
|
|
|
@ -1,8 +1,7 @@
|
||||||
module Groups
|
module Groups
|
||||||
class DestroyService < Groups::BaseService
|
class DestroyService < Groups::BaseService
|
||||||
def async_execute
|
def async_execute
|
||||||
# Soft delete via paranoia gem
|
group.soft_delete_without_removing_associations
|
||||||
group.destroy
|
|
||||||
job_id = GroupDestroyWorker.perform_async(group.id, current_user.id)
|
job_id = GroupDestroyWorker.perform_async(group.id, current_user.id)
|
||||||
Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}")
|
Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}")
|
||||||
end
|
end
|
||||||
|
|
|
@ -0,0 +1,4 @@
|
||||||
|
---
|
||||||
|
title: Defer project destroys within a namespace in Groups::DestroyService#async_execute
|
||||||
|
merge_request:
|
||||||
|
author:
|
|
@ -342,6 +342,17 @@ describe Namespace, models: true do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
describe '#soft_delete_without_removing_associations' do
|
||||||
|
let(:project1) { create(:project_empty_repo, namespace: namespace) }
|
||||||
|
|
||||||
|
it 'updates the deleted_at timestamp but preserves projects' do
|
||||||
|
namespace.soft_delete_without_removing_associations
|
||||||
|
|
||||||
|
expect(Project.all).to include(project1)
|
||||||
|
expect(namespace.deleted_at).not_to be_nil
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
describe '#user_ids_for_project_authorizations' do
|
describe '#user_ids_for_project_authorizations' do
|
||||||
it 'returns the user IDs for which to refresh authorizations' do
|
it 'returns the user IDs for which to refresh authorizations' do
|
||||||
expect(namespace.user_ids_for_project_authorizations)
|
expect(namespace.user_ids_for_project_authorizations)
|
||||||
|
|
|
@ -15,6 +15,14 @@ describe Groups::DestroyService, services: true do
|
||||||
group.add_user(user, Gitlab::Access::OWNER)
|
group.add_user(user, Gitlab::Access::OWNER)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def destroy_group(group, user, async)
|
||||||
|
if async
|
||||||
|
Groups::DestroyService.new(group, user).async_execute
|
||||||
|
else
|
||||||
|
Groups::DestroyService.new(group, user).execute
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
shared_examples 'group destruction' do |async|
|
shared_examples 'group destruction' do |async|
|
||||||
context 'database records' do
|
context 'database records' do
|
||||||
before do
|
before do
|
||||||
|
@ -30,30 +38,14 @@ describe Groups::DestroyService, services: true do
|
||||||
context 'file system' do
|
context 'file system' do
|
||||||
context 'Sidekiq inline' do
|
context 'Sidekiq inline' do
|
||||||
before do
|
before do
|
||||||
# Run sidekiq immediatly to check that renamed dir will be removed
|
# Run sidekiq immediately to check that renamed dir will be removed
|
||||||
Sidekiq::Testing.inline! { destroy_group(group, user, async) }
|
Sidekiq::Testing.inline! { destroy_group(group, user, async) }
|
||||||
end
|
end
|
||||||
|
|
||||||
it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
|
it 'verifies that paths have been deleted' do
|
||||||
it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey }
|
expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey
|
||||||
|
expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'Sidekiq fake' do
|
|
||||||
before do
|
|
||||||
# Don't run sidekiq to check if renamed repository exists
|
|
||||||
Sidekiq::Testing.fake! { destroy_group(group, user, async) }
|
|
||||||
end
|
|
||||||
|
|
||||||
it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
|
|
||||||
it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy }
|
|
||||||
end
|
|
||||||
end
|
|
||||||
|
|
||||||
def destroy_group(group, user, async)
|
|
||||||
if async
|
|
||||||
Groups::DestroyService.new(group, user).async_execute
|
|
||||||
else
|
|
||||||
Groups::DestroyService.new(group, user).execute
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
@ -61,6 +53,26 @@ describe Groups::DestroyService, services: true do
|
||||||
describe 'asynchronous delete' do
|
describe 'asynchronous delete' do
|
||||||
it_behaves_like 'group destruction', true
|
it_behaves_like 'group destruction', true
|
||||||
|
|
||||||
|
context 'Sidekiq fake' do
|
||||||
|
before do
|
||||||
|
# Don't run Sidekiq to verify that group and projects are not actually destroyed
|
||||||
|
Sidekiq::Testing.fake! { destroy_group(group, user, true) }
|
||||||
|
end
|
||||||
|
|
||||||
|
after do
|
||||||
|
# Clean up stale directories
|
||||||
|
gitlab_shell.rm_namespace(project.repository_storage_path, group.path)
|
||||||
|
gitlab_shell.rm_namespace(project.repository_storage_path, remove_path)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'verifies original paths and projects still exist' do
|
||||||
|
expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_truthy
|
||||||
|
expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey
|
||||||
|
expect(Project.unscoped.count).to eq(1)
|
||||||
|
expect(Group.unscoped.count).to eq(2)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
context 'potential race conditions' do
|
context 'potential race conditions' do
|
||||||
context "when the `GroupDestroyWorker` task runs immediately" do
|
context "when the `GroupDestroyWorker` task runs immediately" do
|
||||||
it "deletes the group" do
|
it "deletes the group" do
|
||||||
|
|
Loading…
Reference in a new issue