From 61cfd1d2733a717934a723d36f60e7bcd09fad05 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Jun 2015 14:07:20 +0200 Subject: Wrap group removal into service Signed-off-by: Dmitriy Zaporozhets --- app/controllers/admin/groups_controller.rb | 2 +- app/controllers/groups_controller.rb | 2 +- app/services/destroy_group_service.rb | 11 +++++++++++ lib/api/groups.rb | 2 +- 4 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 app/services/destroy_group_service.rb diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 2dfae13ac5c..4d3e48f7f81 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -47,7 +47,7 @@ class Admin::GroupsController < Admin::ApplicationController end def destroy - @group.destroy + DestroyGroupService.new(@group, current_user).execute redirect_to admin_groups_path, notice: 'Group was successfully deleted.' end diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 34f0b257db3..2e381822e42 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -82,7 +82,7 @@ class GroupsController < Groups::ApplicationController end def destroy - @group.destroy + DestroyGroupService.new(@group, current_user).execute redirect_to root_path, notice: 'Group was removed.' end diff --git a/app/services/destroy_group_service.rb b/app/services/destroy_group_service.rb new file mode 100644 index 00000000000..9637a1480a8 --- /dev/null +++ b/app/services/destroy_group_service.rb @@ -0,0 +1,11 @@ +class DestroyGroupService + attr_accessor :group, :current_user + + def initialize(group, user) + @group, @current_user = group, user + end + + def execute + @group.destroy + end +end diff --git a/lib/api/groups.rb b/lib/api/groups.rb index f768c750402..e88b6e31775 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -62,7 +62,7 @@ module API delete ":id" do group = find_group(params[:id]) authorize! :admin_group, group - group.destroy + DestroyGroupService.new(group, current_user).execute end # Transfer a project to the Group namespace -- cgit v1.2.1 From 1edff53444ea493ee010a83220cf13ccb381b411 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Jun 2015 14:57:12 +0200 Subject: Remove projects before group/user. Remove namespace directory async Signed-off-by: Dmitriy Zaporozhets --- app/models/namespace.rb | 11 +++++++++-- app/services/delete_user_service.rb | 5 +++++ app/services/destroy_group_service.rb | 5 +++++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 211dfa76b81..8918e4a682e 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -72,7 +72,7 @@ class Namespace < ActiveRecord::Base path.gsub!(/[^a-zA-Z0-9_\-\.]/, "") # Users with the great usernames of "." or ".." would end up with a blank username. - # Work around that by setting their username to "blank", followed by a counter. + # Work around that by setting their username to "blank", followed by a counter. path = "blank" if path.blank? counter = 0 @@ -99,7 +99,14 @@ class Namespace < ActiveRecord::Base end def rm_dir - gitlab_shell.rm_namespace(path) + # Move namespace directory into trash. + # We will remove it later async + new_path = "#{path}+#{id}+deleted" + gitlab_shell.mv_namespace(path, new_path) + + # Remove namespace directroy async with delay so + # GitLab has time to remove all projects first + GitlabShellWorker.perform_in(5.minutes, :rm_namespace, new_path) end def move_dir diff --git a/app/services/delete_user_service.rb b/app/services/delete_user_service.rb index d259b4efca6..ca350eb2a8d 100644 --- a/app/services/delete_user_service.rb +++ b/app/services/delete_user_service.rb @@ -4,6 +4,11 @@ class DeleteUserService user.errors[:base] << 'You must transfer ownership or delete groups before you can remove user' user else + # TODO: Skip remove repository so Namespace#rm_dir works + user.personal_projects.each do |project| + ::Projects::DestroyService.new(project, current_user, {}).execute + end + user.destroy end end diff --git a/app/services/destroy_group_service.rb b/app/services/destroy_group_service.rb index 9637a1480a8..c1add7b92e8 100644 --- a/app/services/destroy_group_service.rb +++ b/app/services/destroy_group_service.rb @@ -6,6 +6,11 @@ class DestroyGroupService end def execute + # TODO: Skip remove repository so Namespace#rm_dir works + @group.projects.each do |project| + ::Projects::DestroyService.new(project, current_user, {}).execute + end + @group.destroy end end -- cgit v1.2.1 From 47a95754de18a8f08aa78bf9f20223f263fa8c90 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Jun 2015 16:15:58 +0200 Subject: Log group creation and removal Signed-off-by: Dmitriy Zaporozhets --- app/models/group.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/group.rb b/app/models/group.rb index b4e908c5602..051c672cb33 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -101,10 +101,14 @@ class Group < Namespace end def post_create_hook + Gitlab::AppLogger.info("Group \"#{name}\" was created") + system_hook_service.execute_hooks_for(self, :create) end def post_destroy_hook + Gitlab::AppLogger.info("Group \"#{name}\" was removed") + system_hook_service.execute_hooks_for(self, :destroy) end -- cgit v1.2.1 From 53a0ac47344b9b1d93ec36d9e6b41e2394598beb Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Jun 2015 16:16:27 +0200 Subject: Skip repo removing whem remove user or group Signed-off-by: Dmitriy Zaporozhets --- app/models/namespace.rb | 12 +++++--- app/services/delete_user_service.rb | 5 ++-- app/services/destroy_group_service.rb | 5 ++-- app/services/projects/destroy_service.rb | 8 ++++-- spec/services/destroy_group_service_spec.rb | 44 +++++++++++++++++++++++++++++ 5 files changed, 63 insertions(+), 11 deletions(-) create mode 100644 spec/services/destroy_group_service_spec.rb diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 8918e4a682e..03d2ab165ea 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -102,11 +102,15 @@ class Namespace < ActiveRecord::Base # Move namespace directory into trash. # We will remove it later async new_path = "#{path}+#{id}+deleted" - gitlab_shell.mv_namespace(path, new_path) - # Remove namespace directroy async with delay so - # GitLab has time to remove all projects first - GitlabShellWorker.perform_in(5.minutes, :rm_namespace, new_path) + if gitlab_shell.mv_namespace(path, new_path) + message = "Namespace directory \"#{path}\" moved to \"#{new_path}\"" + Gitlab::AppLogger.info message + + # Remove namespace directroy async with delay so + # GitLab has time to remove all projects first + GitlabShellWorker.perform_in(5.minutes, :rm_namespace, new_path) + end end def move_dir diff --git a/app/services/delete_user_service.rb b/app/services/delete_user_service.rb index ca350eb2a8d..9017a63af3b 100644 --- a/app/services/delete_user_service.rb +++ b/app/services/delete_user_service.rb @@ -4,9 +4,10 @@ class DeleteUserService user.errors[:base] << 'You must transfer ownership or delete groups before you can remove user' user else - # TODO: Skip remove repository so Namespace#rm_dir works user.personal_projects.each do |project| - ::Projects::DestroyService.new(project, current_user, {}).execute + # Skip repository removal because we remove directory with namespace + # that contain all this repositories + ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute end user.destroy diff --git a/app/services/destroy_group_service.rb b/app/services/destroy_group_service.rb index c1add7b92e8..d929a676293 100644 --- a/app/services/destroy_group_service.rb +++ b/app/services/destroy_group_service.rb @@ -6,9 +6,10 @@ class DestroyGroupService end def execute - # TODO: Skip remove repository so Namespace#rm_dir works @group.projects.each do |project| - ::Projects::DestroyService.new(project, current_user, {}).execute + # Skip repository removal because we remove directory with namespace + # that contain all this repositories + ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute end @group.destroy diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 29e8ba347d4..403f419ec50 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -36,9 +36,11 @@ module Projects private def remove_repository(path) - unless gitlab_shell.exists?(path + '.git') - return true - end + # Skip repository removal. We use this flag when remove user or group + return true if params[:skip_repo] == true + + # There is a possibility project does not have repository or wiki + return true unless gitlab_shell.exists?(path + '.git') new_path = removal_path(path) diff --git a/spec/services/destroy_group_service_spec.rb b/spec/services/destroy_group_service_spec.rb new file mode 100644 index 00000000000..24e439503e7 --- /dev/null +++ b/spec/services/destroy_group_service_spec.rb @@ -0,0 +1,44 @@ +require 'spec_helper' + +describe DestroyGroupService do + let!(:user) { create(:user) } + let!(:group) { create(:group) } + let!(:project) { create(:project, namespace: group) } + let!(:gitlab_shell) { Gitlab::Shell.new } + let!(:remove_path) { group.path + "+#{group.id}+deleted" } + + context 'database records' do + before do + destroy_group(group, user) + end + + it { Group.all.should_not include(group) } + it { Project.all.should_not include(project) } + end + + context 'file system' do + context 'Sidekiq inline' do + before do + # Run sidekiq immediatly to check that renamed dir will be removed + Sidekiq::Testing.inline! { destroy_group(group, user) } + end + + it { gitlab_shell.exists?(group.path).should be_falsey } + it { gitlab_shell.exists?(remove_path).should be_falsey } + end + + context 'Sidekiq fake' do + before do + # Dont run sidekiq to check if renamed repository exists + Sidekiq::Testing.fake! { destroy_group(group, user) } + end + + it { gitlab_shell.exists?(group.path).should be_falsey } + it { gitlab_shell.exists?(remove_path).should be_truthy } + end + end + + def destroy_group(group, user) + DestroyGroupService.new(group, user).execute + end +end -- cgit v1.2.1 From 55715735d10c3c35e52af6b67c99dcdbb5c7bf97 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Jun 2015 17:01:31 +0200 Subject: Add CHANGELOG item Signed-off-by: Dmitriy Zaporozhets --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 455b4dcf960..cbd6d92e130 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -42,6 +42,7 @@ v 7.12.0 (unreleased) - Better performance for web editor (switched from satellites to rugged) - GitLab CI service sends .gitlab-ci.yaml in each push call - When remove project - move repository and schedule it removal + - Improve group removing logic v 7.11.4 - Fix missing bullets when creating lists -- cgit v1.2.1