Merge branch '55230-remove-project-cleanup-feature-flag' into 'master'

Remove the project_cleanup feature flag

Closes #55230

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

app/controllers/projects/settings/repository_controller.rb

@@ -5,7 +5,6 @@ module Projects
     class RepositoryController < Projects::ApplicationController
       before_action :authorize_admin_project!
       before_action :remote_mirror, only: [:show]
-      before_action :check_cleanup_feature_flag!, only: :cleanup
 
       def show
         render_show
@@ -37,10 +36,6 @@ module Projects
 
       private
 
-      def check_cleanup_feature_flag!
-        render_404 unless ::Feature.enabled?(:project_cleanup, project)
-      end
-
       def render_show
         @deploy_keys = DeployKeysPresenter.new(@project, current_user: current_user)
         @deploy_tokens = @project.deploy_tokens.active

app/views/projects/cleanup/_show.html.haml

-- return unless Feature.enabled?(:project_cleanup, @project)
-
 - expanded = Rails.env.test?
 
 %section.settings.no-animate#cleanup{ class: ('expanded' if expanded) }

lib/gitlab/gitaly_client/cleanup_service.rb

@@ -20,6 +20,7 @@ module Gitlab
 
           while data = io.read(RepositoryService::MAX_MSG_SIZE)
             y.yield Gitaly::ApplyBfgObjectMapRequest.new(object_map: data)
+            break if io&.eof?
           end
         end
 

spec/controllers/projects/settings/repository_controller_spec.rb

@@ -19,35 +19,15 @@ describe Projects::Settings::RepositoryController do
   end
 
   describe 'PUT cleanup' do
-    before do
-      allow(RepositoryCleanupWorker).to receive(:perform_async)
-    end
+    let(:object_map) { fixture_file_upload('spec/fixtures/bfg_object_map.txt') }
 
-    def do_put!
-      object_map = fixture_file_upload('spec/fixtures/bfg_object_map.txt')
+    it 'enqueues a RepositoryCleanupWorker' do
+      allow(RepositoryCleanupWorker).to receive(:perform_async)
 
       put :cleanup, namespace_id: project.namespace, project_id: project, project: { object_map: object_map }
-    end
-
-    context 'feature enabled' do
-      it 'enqueues a RepositoryCleanupWorker' do
-        stub_feature_flags(project_cleanup: true)
-
-        do_put!
-
-        expect(response).to redirect_to project_settings_repository_path(project)
-        expect(RepositoryCleanupWorker).to have_received(:perform_async).once
-      end
-    end
-
-    context 'feature disabled' do
-      it 'shows a 404 error' do
-        stub_feature_flags(project_cleanup: false)
-
-        do_put!
 
-        expect(response).to have_gitlab_http_status(404)
-      end
+      expect(response).to redirect_to project_settings_repository_path(project)
+      expect(RepositoryCleanupWorker).to have_received(:perform_async).once
     end
   end
 end

spec/features/projects/settings/repository_settings_spec.rb

@@ -200,35 +200,21 @@ describe 'Projects > Settings > Repository settings' do
     context 'repository cleanup settings' do
       let(:object_map_file) { Rails.root.join('spec', 'fixtures', 'bfg_object_map.txt') }
 
-      context 'feature enabled' do
-        it 'uploads an object map file', :js do
-          stub_feature_flags(project_cleanup: true)
-
-          visit project_settings_repository_path(project)
+      it 'uploads an object map file', :js do
+        visit project_settings_repository_path(project)
 
-          expect(page).to have_content('Repository cleanup')
+        expect(page).to have_content('Repository cleanup')
 
-          page.within('#cleanup') do
-            attach_file('project[bfg_object_map]', object_map_file, visible: false)
+        page.within('#cleanup') do
+          attach_file('project[bfg_object_map]', object_map_file, visible: false)
 
-            Sidekiq::Testing.fake! do
-              click_button 'Start cleanup'
-            end
+          Sidekiq::Testing.fake! do
+            click_button 'Start cleanup'
           end
-
-          expect(page).to have_content('Repository cleanup has started')
-          expect(RepositoryCleanupWorker.jobs.count).to eq(1)
         end
-      end
 
-      context 'feature disabled' do
-        it 'does not show the settings' do
-          stub_feature_flags(project_cleanup: false)
-
-          visit project_settings_repository_path(project)
-
-          expect(page).not_to have_content('Repository cleanup')
-        end
+        expect(page).to have_content('Repository cleanup has started')
+        expect(RepositoryCleanupWorker.jobs.count).to eq(1)
       end
     end
   end

spec/lib/gitlab/git/repository_cleaner_spec.rb

 require 'spec_helper'
 
 describe Gitlab::Git::RepositoryCleaner do
+  include HttpIOHelpers
+
   let(:project) { create(:project, :repository) }
   let(:repository) { project.repository }
   let(:head_sha) { repository.head_commit.id }
-
-  let(:object_map) { StringIO.new("#{head_sha} #{'0' * 40}") }
+  let(:object_map_data) { "#{head_sha} #{'0' * 40}" }
 
   subject(:cleaner) { described_class.new(repository.raw) }
 
   describe '#apply_bfg_object_map' do
-    it 'removes internal references pointing at SHAs in the object map' do
-      # Create some refs we expect to be removed
-      repository.keep_around(head_sha)
-      repository.create_ref(head_sha, 'refs/environments/1')
-      repository.create_ref(head_sha, 'refs/merge-requests/1')
-      repository.create_ref(head_sha, 'refs/heads/_keep')
-      repository.create_ref(head_sha, 'refs/tags/_keep')
-
-      cleaner.apply_bfg_object_map(object_map)
-
-      aggregate_failures do
-        expect(repository.kept_around?(head_sha)).to be_falsy
-        expect(repository.ref_exists?('refs/environments/1')).to be_falsy
-        expect(repository.ref_exists?('refs/merge-requests/1')).to be_falsy
-        expect(repository.ref_exists?('refs/heads/_keep')).to be_truthy
-        expect(repository.ref_exists?('refs/tags/_keep')).to be_truthy
+    let(:clean_refs) { %W[refs/environments/1 refs/merge-requests/1 refs/keep-around/#{head_sha}] }
+    let(:keep_refs) { %w[refs/heads/_keep refs/tags/_keep] }
+
+    before do
+      (clean_refs + keep_refs).each { |ref| repository.create_ref(head_sha, ref) }
+    end
+
+    context 'from StringIO' do
+      let(:object_map) { StringIO.new(object_map_data) }
+
+      it 'removes internal references' do
+        cleaner.apply_bfg_object_map(object_map)
+
+        aggregate_failures do
+          clean_refs.each { |ref| expect(repository.ref_exists?(ref)).to be_falsy }
+          keep_refs.each { |ref| expect(repository.ref_exists?(ref)).to be_truthy }
+        end
+      end
+    end
+
+    context 'from Gitlab::HttpIO' do
+      let(:url) { 'http://example.com/bfg_object_map.txt' }
+      let(:tempfile) { Tempfile.new }
+      let(:object_map) { Gitlab::HttpIO.new(url, object_map_data.size) }
+
+      around do |example|
+        begin
+          tempfile.write(object_map_data)
+          tempfile.close
+
+          example.run
+        ensure
+          tempfile.unlink
+        end
+      end
+
+      it 'removes internal references' do
+        stub_remote_url_200(url, tempfile.path)
+
+        cleaner.apply_bfg_object_map(object_map)
+
+        aggregate_failures do
+          clean_refs.each { |ref| expect(repository.ref_exists?(ref)).to be_falsy }
+          keep_refs.each { |ref| expect(repository.ref_exists?(ref)).to be_truthy }
+        end
       end
     end
   end