diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index b1f77bf242cc8e17a484f07f44003ed79b762bce..7c21284b99cce06879030dfa1ced5b28eba54e7b 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -41,7 +41,7 @@ module Ci before_save :update_artifacts_size, if: :artifacts_file_changed? before_save :ensure_token - before_destroy { project } + before_destroy { unscoped_project } after_create :execute_hooks after_save :update_project_statistics, if: :artifacts_size_changed? @@ -416,16 +416,23 @@ module Ci # This method returns old path to artifacts only if it already exists. # def artifacts_path + # We need the project even if it's soft deleted, because whenever + # we're really deleting the project, we'll also delete the builds, + # and in order to delete the builds, we need to know where to find + # the artifacts, which is depending on the data of the project. + # We need to retain the project in this case. + the_project = project || unscoped_project + old = File.join(created_at.utc.strftime('%Y_%m'), - project.ci_id.to_s, + the_project.ci_id.to_s, id.to_s) old_store = File.join(ArtifactUploader.artifacts_path, old) - return old if project.ci_id && File.directory?(old_store) + return old if the_project.ci_id && File.directory?(old_store) File.join( created_at.utc.strftime('%Y_%m'), - project.id.to_s, + the_project.id.to_s, id.to_s ) end @@ -559,6 +566,10 @@ module Ci self.update(erased_by: user, erased_at: Time.now, artifacts_expire_at: nil) end + def unscoped_project + @unscoped_project ||= Project.unscoped.find_by(id: gl_project_id) + end + def predefined_variables variables = [ { key: 'CI', value: 'true', public: true }, @@ -597,6 +608,8 @@ module Ci end def update_project_statistics + return unless project + ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size]) end end diff --git a/changelogs/unreleased/fix-deleting-project-again.yml b/changelogs/unreleased/fix-deleting-project-again.yml new file mode 100644 index 0000000000000000000000000000000000000000..e13215f22a75235af9cf5fead67a6b515c146e5f --- /dev/null +++ b/changelogs/unreleased/fix-deleting-project-again.yml @@ -0,0 +1,4 @@ +--- +title: Fix deleting projects with pipelines and builds +merge_request: 8960 +author: diff --git a/spec/workers/project_destroy_worker_spec.rb b/spec/workers/project_destroy_worker_spec.rb index 1f4c39eb64ac7b7d6748e8f2409104132fae8588..4b1a342930c1ac8ecac71e20f2c5692f14d32e9a 100644 --- a/spec/workers/project_destroy_worker_spec.rb +++ b/spec/workers/project_destroy_worker_spec.rb @@ -1,21 +1,28 @@ require 'spec_helper' describe ProjectDestroyWorker do - let(:project) { create(:project) } + let(:project) { create(:project, pending_delete: true) } let(:path) { project.repository.path_to_repo } subject { ProjectDestroyWorker.new } - describe "#perform" do - it "deletes the project" do - subject.perform(project.id, project.owner.id, {}) + describe '#perform' do + context 'with pipelines and builds' do + let!(:pipeline) { create(:ci_pipeline, project: project) } + let!(:build) { create(:ci_build, :artifacts, pipeline: pipeline) } - expect(Project.all).not_to include(project) - expect(Dir.exist?(path)).to be_falsey + it 'deletes the project along with pipelines and builds' do + subject.perform(project.id, project.owner.id, {}) + + expect(Project.all).not_to include(project) + expect(Ci::Pipeline.all).not_to include(pipeline) + expect(Ci::Build.all).not_to include(build) + expect(Dir.exist?(path)).to be_falsey + end end - it "deletes the project but skips repo deletion" do - subject.perform(project.id, project.owner.id, { "skip_repo" => true }) + it 'deletes the project but skips repo deletion' do + subject.perform(project.id, project.owner.id, { 'skip_repo' => true }) expect(Project.all).not_to include(project) expect(Dir.exist?(path)).to be_truthy