diff options
| -rw-r--r-- | app/services/projects/destroy_service.rb | 9 | ||||
| -rw-r--r-- | app/workers/project_destroy_worker.rb | 3 | ||||
| -rw-r--r-- | db/migrate/20170428064307_add_column_delete_error_to_projects.rb | 31 | ||||
| -rw-r--r-- | db/schema.rb | 1 | ||||
| -rw-r--r-- | spec/lib/gitlab/import_export/safe_model_attributes.yml | 1 | ||||
| -rw-r--r-- | spec/services/projects/destroy_service_spec.rb | 63 |
6 files changed, 104 insertions, 4 deletions
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index e2b2660ea71..682407ac896 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -26,9 +26,6 @@ module Projects Projects::UnlinkForkService.new(project, current_user).execute Project.transaction do - project.team.truncate - project.destroy! - unless remove_legacy_registry_tags raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.') end @@ -40,10 +37,14 @@ module Projects unless remove_repository(wiki_path) raise_error('Failed to remove wiki repository. Please try again or contact administrator.') end + + project.team.truncate + project.destroy! end - log_info("Project \"#{project.path_with_namespace}\" was removed") system_hook_service.execute_hooks_for(project, :destroy) + + log_info("Project \"#{project.path_with_namespace}\" was removed") true end diff --git a/app/workers/project_destroy_worker.rb b/app/workers/project_destroy_worker.rb index b462327490e..482e1e38cd1 100644 --- a/app/workers/project_destroy_worker.rb +++ b/app/workers/project_destroy_worker.rb @@ -12,5 +12,8 @@ class ProjectDestroyWorker user = User.find(user_id) ::Projects::DestroyService.new(project, user, params.symbolize_keys).execute + rescue StandardError => error + project.assign_attributes(delete_error: error.message, pending_delete: false) + project.save!(validate: false) end end diff --git a/db/migrate/20170428064307_add_column_delete_error_to_projects.rb b/db/migrate/20170428064307_add_column_delete_error_to_projects.rb new file mode 100644 index 00000000000..ef5fc2cdea5 --- /dev/null +++ b/db/migrate/20170428064307_add_column_delete_error_to_projects.rb @@ -0,0 +1,31 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddColumnDeleteErrorToProjects < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + # When a migration requires downtime you **must** uncomment the following + # constant and define a short and easy to understand explanation as to why the + # migration requires downtime. + # DOWNTIME_REASON = '' + + # When using the methods "add_concurrent_index", "remove_concurrent_index" or + # "add_column_with_default" you must disable the use of transactions + # as these methods can not run in an existing transaction. + # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure + # that either of them is the _only_ method called in the migration, + # any other changes should go in a separate migration. + # This ensures that upon failure _only_ the index creation or removing fails + # and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + # disable_ddl_transaction! + + def change + add_column :projects, :delete_error, :text + end +end diff --git a/db/schema.rb b/db/schema.rb index 284b2068166..0ba2bd31517 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1134,6 +1134,7 @@ ActiveRecord::Schema.define(version: 20170717150329) do t.integer "cached_markdown_version" t.datetime "last_repository_updated_at" t.string "ci_config_path" + t.text "delete_error" end add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 4ef3db3721f..0f2db3380a7 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -396,6 +396,7 @@ Project: - build_allow_git_fetch - last_repository_updated_at - ci_config_path +- delete_error Author: - name ProjectFeature: diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index b399d3402fd..a629afe723d 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -36,6 +36,27 @@ describe Projects::DestroyService, services: true do end end + shared_examples 'handles errors thrown during async destroy' do |error_message| + it 'does not allow the error to bubble up' do + expect do + Sidekiq::Testing.inline! { destroy_project(project, user, {}) } + end.not_to raise_error + end + + it 'unmarks the project as "pending deletion"' do + Sidekiq::Testing.inline! { destroy_project(project, user, {}) } + + expect(project.reload.pending_delete).to be(false) + end + + it 'stores an error message in `projects.delete_error`' do + Sidekiq::Testing.inline! { destroy_project(project, user, {}) } + + expect(project.reload.delete_error).to be_present + expect(project.delete_error).to include(error_message) + end + end + context 'Sidekiq inline' do before do # Run sidekiq immediatly to check that renamed repository will be removed @@ -89,10 +110,52 @@ describe Projects::DestroyService, services: true do end it_behaves_like 'deleting the project with pipeline and build' + + context 'errors' do + context 'when `remove_legacy_registry_tags` fails' do + before do + expect_any_instance_of(Projects::DestroyService) + .to receive(:remove_legacy_registry_tags).and_return(false) + end + + it_behaves_like 'handles errors thrown during async destroy', "Failed to remove some tags" + end + + context 'when `remove_repository` fails' do + before do + expect_any_instance_of(Projects::DestroyService) + .to receive(:remove_repository).and_return(false) + end + + it_behaves_like 'handles errors thrown during async destroy', "Failed to remove project repository" + end + + context 'when `execute` raises any other error' do + before do + expect_any_instance_of(Projects::DestroyService) + .to receive(:execute).and_raise(ArgumentError.new("Other error message")) + end + + it_behaves_like 'handles errors thrown during async destroy', "Other error message" + end + end end context 'with execute' do it_behaves_like 'deleting the project with pipeline and build' + + context 'when `execute` raises an error' do + before do + expect_any_instance_of(Projects::DestroyService) + .to receive(:execute).and_raise(ArgumentError) + end + + it 'allows the error to bubble up' do + expect do + Sidekiq::Testing.inline! { Projects::DestroyService.new(project, user, {}).execute } + end.to raise_error(ArgumentError) + end + end end describe 'container registry' do |