Merge branch '45481-sane-pages-artifacts' into 'master'

Don't automatically remove artifacts for pages jobs after pages:deploy has run

Closes #45481

See merge request gitlab-org/gitlab-ce!18628

app/services/projects/update_pages_service.rb

 module Projects
   class UpdatePagesService < BaseService
-    InvaildStateError = Class.new(StandardError)
+    InvalidStateError = Class.new(StandardError)
     FailedToExtractError = Class.new(StandardError)
 
     BLOCK_SIZE = 32.kilobytes
@@ -21,8 +21,8 @@ module Projects
       @status.enqueue!
       @status.run!
 
-      raise InvaildStateError, 'missing pages artifacts' unless build.artifacts?
-      raise InvaildStateError, 'pages are outdated' unless latest?
+      raise InvalidStateError, 'missing pages artifacts' unless build.artifacts?
+      raise InvalidStateError, 'pages are outdated' unless latest?
 
       # Create temporary directory in which we will extract the artifacts
       FileUtils.mkdir_p(tmp_path)
@@ -31,16 +31,16 @@ module Projects
 
         # Check if we did extract public directory
         archive_public_path = File.join(archive_path, 'public')
-        raise InvaildStateError, 'pages miss the public folder' unless Dir.exist?(archive_public_path)
-        raise InvaildStateError, 'pages are outdated' unless latest?
+        raise InvalidStateError, 'pages miss the public folder' unless Dir.exist?(archive_public_path)
+        raise InvalidStateError, 'pages are outdated' unless latest?
 
         deploy_page!(archive_public_path)
         success
       end
-    rescue InvaildStateError => e
+    rescue InvalidStateError => e
       error(e.message)
     rescue => e
-      error(e.message, false)
+      error(e.message)
       raise e
     end
 
@@ -48,17 +48,15 @@ module Projects
 
     def success
       @status.success
-      delete_artifact!
       super
     end
 
-    def error(message, allow_delete_artifact = true)
+    def error(message)
       register_failure
       log_error("Projects::UpdatePagesService: #{message}")
       @status.allow_failure = !latest?
       @status.description = message
       @status.drop(:script_failure)
-      delete_artifact! if allow_delete_artifact
       super
     end
 
@@ -77,18 +75,18 @@ module Projects
       if artifacts.ends_with?('.zip')
         extract_zip_archive!(temp_path)
       else
-        raise InvaildStateError, 'unsupported artifacts format'
+        raise InvalidStateError, 'unsupported artifacts format'
       end
     end
 
     def extract_zip_archive!(temp_path)
-      raise InvaildStateError, 'missing artifacts metadata' unless build.artifacts_metadata?
+      raise InvalidStateError, 'missing artifacts metadata' unless build.artifacts_metadata?
 
       # Calculate page size after extract
       public_entry = build.artifacts_metadata_entry(SITE_PATH, recursive: true)
 
       if public_entry.total_size > max_size
-        raise InvaildStateError, "artifacts for pages are too large: #{public_entry.total_size}"
+        raise InvalidStateError, "artifacts for pages are too large: #{public_entry.total_size}"
       end
 
       # Requires UnZip at least 6.00 Info-ZIP.
@@ -162,11 +160,6 @@ module Projects
       build.artifacts_file.path
     end
 
-    def delete_artifact!
-      build.reload # Reload stable object to prevent erase artifacts with old state
-      build.erase_artifacts! unless build.has_expiring_artifacts?
-    end
-
     def latest_sha
       project.commit(build.ref).try(:sha).to_s
     ensure

changelogs/unreleased/45481-sane-pages-artifacts.yml

+---
+title: Don't automatically remove artifacts for pages jobs after pages:deploy has
+  run
+merge_request: 18628
+author:
+type: fixed

spec/services/projects/update_pages_service_spec.rb

@@ -29,25 +29,10 @@ describe Projects::UpdatePagesService do
     end
 
     describe 'pages artifacts' do
-      context 'with expiry date' do
-        before do
-          build.artifacts_expire_in = "2 days"
-          build.save!
-        end
-
-        it "doesn't delete artifacts" do
-          expect(execute).to eq(:success)
-
-          expect(build.reload.artifacts?).to eq(true)
-        end
-      end
-
-      context 'without expiry date' do
-        it "does delete artifacts" do
-          expect(execute).to eq(:success)
+      it "doesn't delete artifacts after deploying" do
+        expect(execute).to eq(:success)
 
-          expect(build.reload.artifacts?).to eq(false)
-        end
+        expect(build.reload.artifacts?).to eq(true)
       end
     end
 
@@ -100,25 +85,10 @@ describe Projects::UpdatePagesService do
       end
 
       describe 'pages artifacts' do
-        context 'with expiry date' do
-          before do
-            build.artifacts_expire_in = "2 days"
-            build.save!
-          end
-
-          it "doesn't delete artifacts" do
-            expect(execute).to eq(:success)
-
-            expect(build.artifacts?).to eq(true)
-          end
-        end
-
-        context 'without expiry date' do
-          it "does delete artifacts" do
-            expect(execute).to eq(:success)
+        it "doesn't delete artifacts after deploying" do
+          expect(execute).to eq(:success)
 
-            expect(build.reload.artifacts?).to eq(false)
-          end
+          expect(build.artifacts?).to eq(true)
         end
       end
 
@@ -171,13 +141,12 @@ describe Projects::UpdatePagesService do
 
           build.reload
           expect(deploy_status).to be_failed
-          expect(build.artifacts?).to be_truthy
         end
       end
 
       context 'when failed to extract zip artifacts' do
         before do
-          allow_any_instance_of(described_class)
+          expect_any_instance_of(described_class)
             .to receive(:extract_zip_archive!)
             .and_raise(Projects::UpdatePagesService::FailedToExtractError)
         end
@@ -188,21 +157,19 @@ describe Projects::UpdatePagesService do
 
           build.reload
           expect(deploy_status).to be_failed
-          expect(build.artifacts?).to be_truthy
         end
       end
 
       context 'when missing artifacts metadata' do
         before do
-          allow(build).to receive(:artifacts_metadata?).and_return(false)
+          expect(build).to receive(:artifacts_metadata?).and_return(false)
         end
 
-        it 'does not raise an error and remove artifacts as failed job' do
+        it 'does not raise an error as failed job' do
           execute
 
           build.reload
           expect(deploy_status).to be_failed
-          expect(build.artifacts?).to be_falsey
         end
       end
     end