diff options
| -rw-r--r-- | CHANGELOG | 1 | ||||
| -rw-r--r-- | app/controllers/projects_controller.rb | 17 | ||||
| -rw-r--r-- | app/services/projects/destroy_service.rb | 65 | ||||
| -rw-r--r-- | lib/gitlab/backend/shell.rb | 14 | ||||
| -rw-r--r-- | spec/features/projects_spec.rb | 7 | ||||
| -rw-r--r-- | spec/requests/api/projects_spec.rb | 9 | ||||
| -rw-r--r-- | spec/services/projects/destroy_service_spec.rb | 34 |
7 files changed, 106 insertions, 41 deletions
diff --git a/CHANGELOG b/CHANGELOG index 85fa933fa6b..455b4dcf960 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -41,6 +41,7 @@ v 7.12.0 (unreleased) - Add an option to automatically sign-in with an Omniauth provider - 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 v 7.11.4 - Fix missing bullets when creating lists diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index dc430351551..4ca5fc65459 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -97,18 +97,15 @@ class ProjectsController < ApplicationController return access_denied! unless can?(current_user, :remove_project, @project) ::Projects::DestroyService.new(@project, current_user, {}).execute + flash[:alert] = 'Project deleted.' - respond_to do |format| - format.html do - flash[:alert] = 'Project deleted.' - - if request.referer.include?('/admin') - redirect_to admin_namespaces_projects_path - else - redirect_to dashboard_path - end - end + if request.referer.include?('/admin') + redirect_to admin_namespaces_projects_path + else + redirect_to dashboard_path end + rescue Projects::DestroyService::DestroyError => ex + redirect_to edit_project_path(@project), alert: ex.message end def autocomplete_sources diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 7e1d753b021..29e8ba347d4 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -1,28 +1,67 @@ module Projects class DestroyService < BaseService + include Gitlab::ShellAdapter + + class DestroyError < StandardError; end + + DELETED_FLAG = '+deleted' + def execute return false unless can?(current_user, :remove_project, project) project.team.truncate project.repository.expire_cache unless project.empty_repo? - if project.destroy - GitlabShellWorker.perform_async( - :remove_repository, - project.path_with_namespace - ) + repo_path = project.path_with_namespace + wiki_path = repo_path + '.wiki' + + Project.transaction do + project.destroy! - GitlabShellWorker.perform_async( - :remove_repository, - project.path_with_namespace + ".wiki" - ) + unless remove_repository(repo_path) + raise_error('Failed to remove project repository. Please try again or contact administrator') + end + + unless remove_repository(wiki_path) + raise_error('Failed to remove wiki repository. Please try again or contact administrator') + end + end + + project.satellite.destroy + log_info("Project \"#{project.name}\" was removed") + system_hook_service.execute_hooks_for(project, :destroy) + true + end - project.satellite.destroy + private - log_info("Project \"#{project.name}\" was removed") - system_hook_service.execute_hooks_for(project, :destroy) - true + def remove_repository(path) + unless gitlab_shell.exists?(path + '.git') + return true end + + new_path = removal_path(path) + + if gitlab_shell.mv_repository(path, new_path) + log_info("Repository \"#{path}\" moved to \"#{new_path}\"") + GitlabShellWorker.perform_in(5.minutes, :remove_repository, new_path) + else + false + end + end + + def raise_error(message) + raise DestroyError.new(message) + end + + # Build a path for removing repositories + # We use `+` because its not allowed by GitLab so user can not create + # project with name cookies+119+deleted and capture someone stalled repository + # + # gitlab/cookies.git -> gitlab/cookies+119+deleted.git + # + def removal_path(path) + "#{path}+#{project.id}#{DELETED_FLAG}" end end end diff --git a/lib/gitlab/backend/shell.rb b/lib/gitlab/backend/shell.rb index 530f9d93de4..172d4902add 100644 --- a/lib/gitlab/backend/shell.rb +++ b/lib/gitlab/backend/shell.rb @@ -244,6 +244,16 @@ module Gitlab end end + # Check if such directory exists in repositories. + # + # Usage: + # exists?('gitlab') + # exists?('gitlab/cookies.git') + # + def exists?(dir_name) + File.exists?(full_path(dir_name)) + end + protected def gitlab_shell_path @@ -264,10 +274,6 @@ module Gitlab File.join(repos_path, dir_name) end - def exists?(dir_name) - File.exists?(full_path(dir_name)) - end - def gitlab_shell_projects_path File.join(gitlab_shell_path, 'bin', 'gitlab-projects') end diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index 56523f6e1a8..f8eea70ec4a 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -47,13 +47,6 @@ feature 'Project' do it 'should remove project' do expect { remove_project }.to change {Project.count}.by(-1) end - - it 'should delete the project from disk' do - expect(GitlabShellWorker).to receive(:perform_async). - with(:remove_repository, /#{project.path_with_namespace}/).twice - - remove_project - end end def remove_project diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 46cd26eb927..dbfd72e5f19 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -57,14 +57,14 @@ describe API::API, api: true do expect(json_response.first['name']).to eq(project.name) expect(json_response.first['owner']['username']).to eq(user.username) end - + it 'should include the project labels as the tag_list' do get api('/projects', user) response.status.should == 200 json_response.should be_an Array json_response.first.keys.should include('tag_list') end - + context 'and using search' do it 'should return searched project' do get api('/projects', user), { search: project.name } @@ -792,11 +792,6 @@ describe API::API, api: true do describe 'DELETE /projects/:id' do context 'when authenticated as user' do it 'should remove project' do - expect(GitlabShellWorker).to( - receive(:perform_async).with(:remove_repository, - /#{project.path_with_namespace}/) - ).twice - delete api("/projects/#{project.id}", user) expect(response.status).to eq(200) end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb new file mode 100644 index 00000000000..cdf576cc0c1 --- /dev/null +++ b/spec/services/projects/destroy_service_spec.rb @@ -0,0 +1,34 @@ +require 'spec_helper' + +describe Projects::DestroyService do + let!(:user) { create(:user) } + let!(:project) { create(:project, namespace: user.namespace) } + let!(:path) { project.repository.path_to_repo } + let!(:remove_path) { path.sub(/\.git\Z/, "+#{project.id}+deleted.git") } + + context 'Sidekiq inline' do + before do + # Run sidekiq immediatly to check that renamed repository will be removed + Sidekiq::Testing.inline! { destroy_project(project, user, {}) } + end + + it { Project.all.should_not include(project) } + it { Dir.exists?(path).should be_falsey } + it { Dir.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_project(project, user, {}) } + end + + it { Project.all.should_not include(project) } + it { Dir.exists?(path).should be_falsey } + it { Dir.exists?(remove_path).should be_truthy } + end + + def destroy_project(project, user, params) + Projects::DestroyService.new(project, user, params).execute + end +end |