diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index 30724de7f6a..ac3004d069f 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/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 diff --git a/app/views/projects/cleanup/_show.html.haml b/app/views/projects/cleanup/_show.html.haml index 778d27fc61d..cecc139b183 100644 --- a/app/views/projects/cleanup/_show.html.haml +++ b/app/views/projects/cleanup/_show.html.haml @@ -1,5 +1,3 @@ -- return unless Feature.enabled?(:project_cleanup, @project) - - expanded = Rails.env.test? %section.settings.no-animate#cleanup{ class: ('expanded' if expanded) } diff --git a/lib/gitlab/gitaly_client/cleanup_service.rb b/lib/gitlab/gitaly_client/cleanup_service.rb index 8e412a9b3ef..3e8d6a773ca 100644 --- a/lib/gitlab/gitaly_client/cleanup_service.rb +++ b/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 diff --git a/spec/controllers/projects/settings/repository_controller_spec.rb b/spec/controllers/projects/settings/repository_controller_spec.rb index 70f79a47e63..1c6ddfc1864 100644 --- a/spec/controllers/projects/settings/repository_controller_spec.rb +++ b/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 diff --git a/spec/features/projects/settings/repository_settings_spec.rb b/spec/features/projects/settings/repository_settings_spec.rb index 418e22f8c35..1982136b89d 100644 --- a/spec/features/projects/settings/repository_settings_spec.rb +++ b/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) + it 'uploads an object map file', :js do + visit project_settings_repository_path(project) - 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 diff --git a/spec/lib/gitlab/git/repository_cleaner_spec.rb b/spec/lib/gitlab/git/repository_cleaner_spec.rb index a9d9e67ef94..7f9cc2bc9ec 100644 --- a/spec/lib/gitlab/git/repository_cleaner_spec.rb +++ b/spec/lib/gitlab/git/repository_cleaner_spec.rb @@ -1,31 +1,61 @@ 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') + 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] } - cleaner.apply_bfg_object_map(object_map) + before do + (clean_refs + keep_refs).each { |ref| repository.create_ref(head_sha, ref) } + end - 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 + 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