Commit fb59f04f authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

Merge branch 'fix-group-remove' into 'master'

Group improvements

* remove projects before removing group
* execute all hooks/events from project destroy when group removed
* log group create/remove
* delay remove of namespace directory (to prevent NFS issues)

Inspired by !759

See merge request !761
parents 8046b697 55715735
......@@ -43,6 +43,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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -99,7 +99,18 @@ 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"
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
......
......@@ -4,6 +4,12 @@ class DeleteUserService
user.errors[:base] << 'You must transfer ownership or delete groups before you can remove user'
user
else
user.personal_projects.each do |project|
# 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
end
end
......
class DestroyGroupService
attr_accessor :group, :current_user
def initialize(group, user)
@group, @current_user = group, user
end
def execute
@group.projects.each do |project|
# 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
end
end
......@@ -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)
......
......@@ -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
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment