Merge branch...

Merge branch '228782-save-artifacts-archive-in-pages_deployment-in-projects-updatepagesservice' into 'master'

Save artifacts archive in pages_deployment in Projects::UpdatePagesService

See merge request gitlab-org/gitlab!42842

app/models/pages_deployment.rb

@@ -11,5 +11,15 @@ class PagesDeployment < ApplicationRecord
   validates :file_store, presence: true, inclusion: { in: ObjectStorage::SUPPORTED_STORES }
   validates :size, presence: true, numericality: { greater_than: 0, only_integer: true }
 
+  before_validation :set_size, if: :file_changed?
+
+  default_value_for(:file_store) { ::Pages::DeploymentUploader.default_store }
+
   mount_file_store_uploader ::Pages::DeploymentUploader
+
+  private
+
+  def set_size
+    self.size = file.size
+  end
 end

app/models/project_pages_metadatum.rb

@@ -5,6 +5,7 @@ class ProjectPagesMetadatum < ApplicationRecord
 
   belongs_to :project, inverse_of: :pages_metadatum
   belongs_to :artifacts_archive, class_name: 'Ci::JobArtifact'
+  belongs_to :pages_deployment
 
   scope :deployed, -> { where(deployed: true) }
 end

app/services/projects/update_pages_service.rb

@@ -97,6 +97,7 @@ module Projects
       build.artifacts_file.use_file do |artifacts_path|
         SafeZip::Extract.new(artifacts_path)
           .extract(directories: [PUBLIC_DIR], to: temp_path)
+        create_pages_deployment(artifacts_path)
       end
     rescue SafeZip::Extract::Error => e
       raise FailedToExtractError, e.message
@@ -118,6 +119,21 @@ module Projects
       FileUtils.rm_r(previous_public_path, force: true)
     end
 
+    def create_pages_deployment(artifacts_path)
+      return unless Feature.enabled?(:zip_pages_deployments, project)
+
+      File.open(artifacts_path) do |file|
+        deployment = project.pages_deployments.create!(file: file)
+        project.pages_metadatum.update!(pages_deployment: deployment)
+      end
+
+      # TODO: schedule old deployment removal https://gitlab.com/gitlab-org/gitlab/-/issues/235730
+    rescue => e
+      # we don't want to break current pages deployment process if something goes wrong
+      # TODO: remove this rescue as part of https://gitlab.com/gitlab-org/gitlab/-/issues/245308
+      Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
+    end
+
     def latest?
       # check if sha for the ref is still the most recent one
       # this helps in case when multiple deployments happens

app/uploaders/pages/deployment_uploader.rb

@@ -19,5 +19,33 @@ module Pages
     def base_dir
       "@hashed"
     end
+
+    # override GitlabUploader
+    # if set to true it erases the original file when uploading
+    # and we copy from the artifacts archive, so artifacts end up
+    # without the file
+    def move_to_cache
+      false
+    end
+
+    class << self
+      # we only upload this files from the rails background job
+      # so we don't need direct upload for pages deployments
+      # this method is here to ignore any user setting
+      def direct_upload_enabled?
+        false
+      end
+
+      # we don't need background uploads because we upload files
+      # to the right store right away, and we already do that in
+      # the background job
+      def background_upload_enabled?
+        false
+      end
+
+      def default_store
+        object_store_enabled? ? ObjectStorage::Store::REMOTE : ObjectStorage::Store::LOCAL
+      end
+    end
   end
 end

config/feature_flags/development/zip_pages_deployments.yml

+---
+name: zip_pages_deployments
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/42834
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/245308
+group: group::release management
+type: development
+default_enabled: false
\ No newline at end of file

spec/factories/pages_deployments.rb

@@ -3,13 +3,11 @@
 FactoryBot.define do
   factory :pages_deployment, class: 'PagesDeployment' do
     project
-    file_store { ObjectStorage::SUPPORTED_STORES.first }
 
     after(:build) do |deployment, _evaluator|
       deployment.file = fixture_file_upload(
         Rails.root.join("spec/fixtures/pages.zip")
       )
-      deployment.size = deployment.file.size
     end
   end
 end

spec/models/pages_deployment_spec.rb

@@ -18,4 +18,21 @@ RSpec.describe PagesDeployment do
       expect(create(:pages_deployment)).to be_valid
     end
   end
+
+  describe 'default for file_store' do
+    it 'uses local store when object storage is not enabled' do
+      expect(build(:pages_deployment).file_store).to eq(ObjectStorage::Store::LOCAL)
+    end
+
+    it 'uses remote store when object storage is enabled' do
+      stub_pages_object_storage(::Pages::DeploymentUploader)
+
+      expect(build(:pages_deployment).file_store).to eq(ObjectStorage::Store::REMOTE)
+    end
+  end
+
+  it 'saves size along with the file' do
+    deployment = create(:pages_deployment)
+    expect(deployment.size).to eq(deployment.file.size)
+  end
 end

spec/services/projects/update_pages_service_spec.rb

@@ -57,6 +57,28 @@ RSpec.describe Projects::UpdatePagesService do
         end
       end
 
+      it 'creates pages_deployment and saves it in the metadata' do
+        expect do
+          expect(execute).to eq(:success)
+        end.to change { project.pages_deployments.count }.by(1)
+
+        deployment = project.pages_deployments.last
+
+        expect(deployment.size).to eq(file.size)
+        expect(deployment.file).to be
+        expect(project.pages_metadatum.reload.pages_deployment_id).to eq(deployment.id)
+      end
+
+      it 'does not create deployment when zip_pages_deployments feature flag is disabled' do
+        stub_feature_flags(zip_pages_deployments: false)
+
+        expect do
+          expect(execute).to eq(:success)
+        end.not_to change { project.pages_deployments.count }
+
+        expect(project.pages_metadatum.reload.pages_deployment_id).to be_nil
+      end
+
       it 'limits pages size' do
         stub_application_setting(max_pages_size: 1)
         expect(execute).not_to eq(:success)

spec/uploaders/pages/deployment_uploader_spec.rb

@@ -6,6 +6,10 @@ RSpec.describe Pages::DeploymentUploader do
   let(:pages_deployment) { create(:pages_deployment) }
   let(:uploader) { described_class.new(pages_deployment, :file) }
 
+  let(:file) do
+    fixture_file_upload("spec/fixtures/pages.zip")
+  end
+
   subject { uploader }
 
   it_behaves_like "builds correct paths",
@@ -18,16 +22,16 @@ RSpec.describe Pages::DeploymentUploader do
       stub_pages_object_storage
     end
 
-    include_context 'with storage', described_class::Store::REMOTE
-
     it_behaves_like 'builds correct paths', store_dir: %r[\A\h{2}/\h{2}/\h{64}/pages_deployments/\d+\z]
-  end
 
-  context 'when file is stored in valid local_path' do
-    let(:file) do
-      fixture_file_upload("spec/fixtures/pages.zip")
+    it 'preserves original file when stores it' do
+      uploader.store!(file)
+
+      expect(File.exist?(file.path)).to be true
     end
+  end
 
+  context 'when file is stored in valid local_path' do
     before do
       uploader.store!(file)
     end
@@ -35,5 +39,21 @@ RSpec.describe Pages::DeploymentUploader do
     subject { uploader.file.path }
 
     it { is_expected.to match(%r[#{uploader.root}/@hashed/\h{2}/\h{2}/\h{64}/pages_deployments/#{pages_deployment.id}/pages.zip]) }
+
+    it 'preserves original file when stores it' do
+      expect(File.exist?(file.path)).to be true
+    end
+  end
+
+  describe '.default_store' do
+    it 'returns local store when object storage is not enabled' do
+      expect(described_class.default_store).to eq(ObjectStorage::Store::LOCAL)
+    end
+
+    it 'returns remote store when object storage is enabled' do
+      stub_pages_object_storage
+
+      expect(described_class.default_store).to eq(ObjectStorage::Store::REMOTE)
+    end
   end
 end