Commit 07b46517 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'fix/import-error-handling' into 'master'

Fix import error handling

Fixes https://gitlab.com/gitlab-com/support-forum/issues/745

This improves import error handling:
- Now if there's an error during importing before the job is scheduled, we also mark the project status as failed.
- Refactored setting the status to failed into one single method.
- Fixed some situations where the error message was missing or simply empty.

See merge request !4366
parents 74849f97 c9e8acd0
...@@ -31,6 +31,7 @@ v 8.9.0 (unreleased) ...@@ -31,6 +31,7 @@ v 8.9.0 (unreleased)
- Cache assigned issue and merge request counts in sidebar nav - Cache assigned issue and merge request counts in sidebar nav
- Cache project build count in sidebar nav - Cache project build count in sidebar nav
- Reduce number of queries needed to render issue labels in the sidebar - Reduce number of queries needed to render issue labels in the sidebar
- Improve error handling importing projects
v 8.8.3 v 8.8.3
- Fix 404 page when viewing TODOs that contain milestones or labels in different projects. !4312 - Fix 404 page when viewing TODOs that contain milestones or labels in different projects. !4312
......
...@@ -1017,4 +1017,16 @@ class Project < ActiveRecord::Base ...@@ -1017,4 +1017,16 @@ class Project < ActiveRecord::Base
builds.running_or_pending.count(:all) builds.running_or_pending.count(:all)
end end
end end
def mark_import_as_failed(error_message)
original_errors = errors.dup
sanitized_message = Gitlab::UrlSanitizer.sanitize(error_message)
import_fail
update_column(:import_error, sanitized_message)
rescue ActiveRecord::ActiveRecordError => e
Rails.logger.error("Error setting import status to failed: #{e.message}. Original error: #{sanitized_message}")
ensure
@errors = original_errors
end
end end
...@@ -56,14 +56,14 @@ module Projects ...@@ -56,14 +56,14 @@ module Projects
after_create_actions if @project.persisted? after_create_actions if @project.persisted?
if @project.errors.empty?
@project.add_import_job if @project.import? @project.add_import_job if @project.import?
else
fail(error: @project.errors.full_messages.join(', '))
end
@project @project
rescue => e rescue => e
message = "Unable to save project: #{e.message}" fail(error: e.message)
Rails.logger.error(message)
@project.errors.add(:base, message) if @project
@project
end end
protected protected
...@@ -103,5 +103,19 @@ module Projects ...@@ -103,5 +103,19 @@ module Projects
end end
end end
end end
def fail(error:)
message = "Unable to save project. Error: #{error}"
message << "Project ID: #{@project.id}" if @project && @project.id
Rails.logger.error(message)
if @project && @project.import?
@project.errors.add(:base, message)
@project.mark_import_as_failed(message)
end
@project
end
end end
end end
...@@ -39,7 +39,7 @@ module Projects ...@@ -39,7 +39,7 @@ module Projects
begin begin
gitlab_shell.import_repository(project.path_with_namespace, project.import_url) gitlab_shell.import_repository(project.path_with_namespace, project.import_url)
rescue Gitlab::Shell::Error => e rescue Gitlab::Shell::Error => e
raise Error, e.message raise Error, "Error importing repository #{project.import_url} into #{project.path_with_namespace} - #{e.message}"
end end
end end
......
...@@ -15,8 +15,7 @@ class RepositoryForkWorker ...@@ -15,8 +15,7 @@ class RepositoryForkWorker
result = gitlab_shell.fork_repository(source_path, target_path) result = gitlab_shell.fork_repository(source_path, target_path)
unless result unless result
logger.error("Unable to fork project #{project_id} for repository #{source_path} -> #{target_path}") logger.error("Unable to fork project #{project_id} for repository #{source_path} -> #{target_path}")
project.update(import_error: "The project could not be forked.") project.mark_import_as_failed('The project could not be forked.')
project.import_fail
return return
end end
...@@ -24,8 +23,7 @@ class RepositoryForkWorker ...@@ -24,8 +23,7 @@ class RepositoryForkWorker
unless project.valid_repo? unless project.valid_repo?
logger.error("Project #{project_id} had an invalid repository after fork") logger.error("Project #{project_id} had an invalid repository after fork")
project.update(import_error: "The forked repository is invalid.") project.mark_import_as_failed('The forked repository is invalid.')
project.import_fail
return return
end end
......
...@@ -13,8 +13,7 @@ class RepositoryImportWorker ...@@ -13,8 +13,7 @@ class RepositoryImportWorker
result = Projects::ImportService.new(project, current_user).execute result = Projects::ImportService.new(project, current_user).execute
if result[:status] == :error if result[:status] == :error
project.update(import_error: Gitlab::UrlSanitizer.sanitize(result[:message])) project.mark_import_as_failed(result[:message])
project.import_fail
return return
end end
......
...@@ -49,7 +49,7 @@ describe Projects::ImportService, services: true do ...@@ -49,7 +49,7 @@ describe Projects::ImportService, services: true do
result = subject.execute result = subject.execute
expect(result[:status]).to eq :error expect(result[:status]).to eq :error
expect(result[:message]).to eq 'Failed to import the repository' expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - Failed to import the repository"
end end
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