Merge branch '54781-store-disk-path-in-database-for-projects-on-legacy-storage' into 'master'

Storage location for projects on legacy storage

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

app/models/namespace.rb

@@ -306,6 +306,7 @@ class Namespace < ActiveRecord::Base
   def write_projects_repository_config
     all_projects.find_each do |project|
       project.write_repository_config
+      project.track_project_repository
     end
   end
 end

app/models/project.rb

@@ -1244,10 +1244,8 @@ class Project < ActiveRecord::Base
   end
 
   def track_project_repository
-    return unless hashed_storage?(:repository)
-
-    project_repo = project_repository || build_project_repository
-    project_repo.update!(shard_name: repository_storage, disk_path: disk_path)
+    repository = project_repository || build_project_repository
+    repository.update!(shard_name: repository_storage, disk_path: disk_path)
   end
 
   def create_repository(force: false)

app/services/projects/after_rename_service.rb

@@ -81,6 +81,7 @@ module Projects
     def update_repository_configuration
       project.reload_repository!
       project.write_repository_config
+      project.track_project_repository
     end
 
     def rename_transferred_documents

app/services/projects/transfer_service.rb

@@ -81,7 +81,7 @@ module Projects
 
         project.old_path_with_namespace = @old_path
 
-        write_repository_config(@new_path)
+        update_repository_configuration(@new_path)
 
         execute_system_hooks
       end
@@ -106,8 +106,9 @@ module Projects
       project.save!
     end
 
-    def write_repository_config(full_path)
+    def update_repository_configuration(full_path)
       project.write_repository_config(gl_full_path: full_path)
+      project.track_project_repository
     end
 
     def refresh_permissions
@@ -123,7 +124,7 @@ module Projects
       rollback_folder_move
       project.reload
       update_namespace_and_visibility(@old_namespace)
-      write_repository_config(@old_path)
+      update_repository_configuration(@old_path)
     end
 
     def rollback_folder_move

spec/models/namespace_spec.rb

@@ -337,32 +337,40 @@ describe Namespace do
       end
     end
 
-    it 'updates project full path in .git/config for each project inside namespace' do
-      parent = create(:group, name: 'mygroup', path: 'mygroup')
-      subgroup = create(:group, name: 'mysubgroup', path: 'mysubgroup', parent: parent)
-      project_in_parent_group = create(:project, :legacy_storage, :repository, namespace: parent, name: 'foo1')
-      hashed_project_in_subgroup = create(:project, :repository, namespace: subgroup, name: 'foo2')
-      legacy_project_in_subgroup = create(:project, :legacy_storage, :repository, namespace: subgroup, name: 'foo3')
-
-      parent.update(path: 'mygroup_new')
-
-      # Routes are loaded when creating the projects, so we need to manually
-      # reload them for the below code to be aware of the above UPDATE.
-      [
-        project_in_parent_group,
-        hashed_project_in_subgroup,
-        legacy_project_in_subgroup
-      ].each do |project|
-        project.route.reload
+    context 'for each project inside the namespace' do
+      let!(:parent) { create(:group, name: 'mygroup', path: 'mygroup') }
+      let!(:subgroup) { create(:group, name: 'mysubgroup', path: 'mysubgroup', parent: parent) }
+      let!(:project_in_parent_group) { create(:project, :legacy_storage, :repository, namespace: parent, name: 'foo1') }
+      let!(:hashed_project_in_subgroup) { create(:project, :repository, namespace: subgroup, name: 'foo2') }
+      let!(:legacy_project_in_subgroup) { create(:project, :legacy_storage, :repository, namespace: subgroup, name: 'foo3') }
+
+      it 'updates project full path in .git/config' do
+        parent.update(path: 'mygroup_new')
+
+        expect(project_rugged(project_in_parent_group).config['gitlab.fullpath']).to eq "mygroup_new/#{project_in_parent_group.path}"
+        expect(project_rugged(hashed_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{hashed_project_in_subgroup.path}"
+        expect(project_rugged(legacy_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{legacy_project_in_subgroup.path}"
       end
 
-      expect(project_rugged(project_in_parent_group).config['gitlab.fullpath']).to eq "mygroup_new/#{project_in_parent_group.path}"
-      expect(project_rugged(hashed_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{hashed_project_in_subgroup.path}"
-      expect(project_rugged(legacy_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{legacy_project_in_subgroup.path}"
-    end
+      it 'updates the project storage location' do
+        repository_project_in_parent_group = create(:project_repository, project: project_in_parent_group)
+        repository_hashed_project_in_subgroup = create(:project_repository, project: hashed_project_in_subgroup)
+        repository_legacy_project_in_subgroup = create(:project_repository, project: legacy_project_in_subgroup)
+
+        parent.update(path: 'mygroup_moved')
+
+        expect(repository_project_in_parent_group.reload.disk_path).to eq "mygroup_moved/#{project_in_parent_group.path}"
+        expect(repository_hashed_project_in_subgroup.reload.disk_path).to eq hashed_project_in_subgroup.disk_path
+        expect(repository_legacy_project_in_subgroup.reload.disk_path).to eq "mygroup_moved/mysubgroup/#{legacy_project_in_subgroup.path}"
+      end
+
+      def project_rugged(project)
+        # Routes are loaded when creating the projects, so we need to manually
+        # reload them for the below code to be aware of the above UPDATE.
+        project.route.reload
 
-    def project_rugged(project)
-      rugged_repo(project.repository)
+        rugged_repo(project.repository)
+      end
     end
   end
 

spec/models/project_spec.rb

@@ -1651,26 +1651,54 @@ describe Project do
   end
 
   describe '#track_project_repository' do
-    let(:project) { create(:project, :repository) }
+    shared_examples 'tracks storage location' do
+      context 'when a project repository entry does not exist' do
+        it 'creates a new entry' do
+          expect { project.track_project_repository }.to change(project, :project_repository)
+        end
+
+        it 'tracks the project storage location' do
+          project.track_project_repository
 
-    it 'creates a project_repository' do
-      project.track_project_repository
+          expect(project.project_repository).to have_attributes(
+            disk_path: project.disk_path,
+            shard_name: project.repository_storage
+          )
+        end
+      end
+
+      context 'when a tracking entry exists' do
+        let!(:project_repository) { create(:project_repository, project: project) }
+        let!(:shard) { create(:shard, name: 'foo') }
+
+        it 'does not create a new entry in the database' do
+          expect { project.track_project_repository }.not_to change(project, :project_repository)
+        end
 
-      expect(project.reload.project_repository).to be_present
-      expect(project.project_repository.disk_path).to eq(project.disk_path)
-      expect(project.project_repository.shard_name).to eq(project.repository_storage)
+        it 'updates the project storage location' do
+          allow(project).to receive(:disk_path).and_return('fancy/new/path')
+          allow(project).to receive(:repository_storage).and_return('foo')
+
+          project.track_project_repository
+
+          expect(project.project_repository).to have_attributes(
+            disk_path: 'fancy/new/path',
+            shard_name: 'foo'
+          )
+        end
+      end
     end
 
-    it 'updates the project_repository' do
-      project.track_project_repository
+    context 'with projects on legacy storage' do
+      let(:project) { create(:project, :repository, :legacy_storage) }
 
-      allow(project).to receive(:disk_path).and_return('@fancy/new/path')
+      it_behaves_like 'tracks storage location'
+    end
 
-      expect do
-        project.track_project_repository
-      end.not_to change(ProjectRepository, :count)
+    context 'with projects on hashed storage' do
+      let(:project) { create(:project, :repository) }
 
-      expect(project.reload.project_repository.disk_path).to eq(project.disk_path)
+      it_behaves_like 'tracks storage location'
     end
   end
 

spec/services/projects/after_rename_service_spec.rb

@@ -99,6 +99,17 @@ describe Projects::AfterRenameService do
 
         expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
       end
+
+      it 'updates storage location' do
+        allow(project_storage).to receive(:rename_repo).and_return(true)
+
+        described_class.new(project).execute
+
+        expect(project.project_repository).to have_attributes(
+          disk_path: project.disk_path,
+          shard_name: project.repository_storage
+        )
+      end
     end
 
     context 'using hashed storage' do
@@ -193,6 +204,15 @@ describe Projects::AfterRenameService do
 
         expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
       end
+
+      it 'updates storage location' do
+        described_class.new(project).execute
+
+        expect(project.project_repository).to have_attributes(
+          disk_path: project.disk_path,
+          shard_name: project.repository_storage
+        )
+      end
     end
   end
 end

spec/services/projects/transfer_service_spec.rb

@@ -63,6 +63,15 @@ describe Projects::TransferService do
       expect(rugged_config['gitlab.fullpath']).to eq "#{group.full_path}/#{project.path}"
     end
 
+    it 'updates storage location' do
+      transfer_project(project, user, group)
+
+      expect(project.project_repository).to have_attributes(
+        disk_path: "#{group.full_path}/#{project.path}",
+        shard_name: project.repository_storage
+      )
+    end
+
     context 'new group has a kubernetes cluster' do
       let(:group_cluster) { create(:cluster, :group, :provided_by_gcp) }
       let(:group) { group_cluster.group }
@@ -139,6 +148,17 @@ describe Projects::TransferService do
         expect(service).not_to receive(:execute_system_hooks)
       end
     end
+
+    it 'does not update storage location' do
+      create(:project_repository, project: project)
+
+      attempt_project_transfer
+
+      expect(project.project_repository).to have_attributes(
+        disk_path: project.disk_path,
+        shard_name: project.repository_storage
+      )
+    end
   end
 
   context 'namespace -> no namespace' do