Merge branch 'bvl-free-system-namespace' into 'master'

Free `system` namespace by moving uploads into `uploads/-`

Closes #33550

See merge request !12917

21 files changed, 144 insertions, 28 deletions

diff --git a/spec/factories/uploads.rb b/spec/factories/uploads.rb
index 1383420fb44..3222c41c3d8 100644
--- a/spec/factories/uploads.rb
+++ b/spec/factories/uploads.rb
@@ -1,7 +1,7 @@
 FactoryGirl.define do
   factory :upload do
     model { build(:project) }
-    path { "uploads/system/project/avatar/avatar.jpg" }
+    path { "uploads/-/system/project/avatar/avatar.jpg" }
     size 100.kilobytes
     uploader "AvatarUploader"
   end
diff --git a/spec/features/admin/admin_appearance_spec.rb b/spec/features/admin/admin_appearance_spec.rb
index 1e2cb8569ec..b9e361328df 100644
--- a/spec/features/admin/admin_appearance_spec.rb
+++ b/spec/features/admin/admin_appearance_spec.rb
@@ -63,11 +63,11 @@ feature 'Admin Appearance', feature: true do
   end
 
   def logo_selector
-    '//img[@src^="/uploads/system/appearance/logo"]'
+    '//img[@src^="/uploads/-/system/appearance/logo"]'
   end
 
   def header_logo_selector
-    '//img[@src^="/uploads/system/appearance/header_logo"]'
+    '//img[@src^="/uploads/-/system/appearance/header_logo"]'
   end
 
   def logo_fixture
diff --git a/spec/features/uploads/user_uploads_avatar_to_group_spec.rb b/spec/features/uploads/user_uploads_avatar_to_group_spec.rb
index 32784de1613..5843f18d89f 100644
--- a/spec/features/uploads/user_uploads_avatar_to_group_spec.rb
+++ b/spec/features/uploads/user_uploads_avatar_to_group_spec.rb
@@ -18,7 +18,7 @@ feature 'User uploads avatar to group', feature: true do
 
     visit group_path(group)
 
-    expect(page).to have_selector(%Q(img[src$="/uploads/system/group/avatar/#{group.id}/dk.png"]))
+    expect(page).to have_selector(%Q(img[src$="/uploads/-/system/group/avatar/#{group.id}/dk.png"]))
 
     # Cheating here to verify something that isn't user-facing, but is important
     expect(group.reload.avatar.file).to exist
diff --git a/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb b/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb
index 82c356735b9..e8171dcaeb0 100644
--- a/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb
+++ b/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb
@@ -16,7 +16,7 @@ feature 'User uploads avatar to profile', feature: true do
 
     visit user_path(user)
 
-    expect(page).to have_selector(%Q(img[src$="/uploads/system/user/avatar/#{user.id}/dk.png"]))
+    expect(page).to have_selector(%Q(img[src$="/uploads/-/system/user/avatar/#{user.id}/dk.png"]))
 
     # Cheating here to verify something that isn't user-facing, but is important
     expect(user.reload.avatar.file).to exist
diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb
index e0cad1da86a..f5e139685e8 100644
--- a/spec/helpers/application_helper_spec.rb
+++ b/spec/helpers/application_helper_spec.rb
@@ -59,13 +59,13 @@ describe ApplicationHelper do
   describe 'project_icon' do
     it 'returns an url for the avatar' do
       project = create(:empty_project, avatar: File.open(uploaded_image_temp_path))
-      avatar_url = "/uploads/system/project/avatar/#{project.id}/banana_sample.gif"
+      avatar_url = "/uploads/-/system/project/avatar/#{project.id}/banana_sample.gif"
 
       expect(helper.project_icon(project.full_path).to_s)
         .to eq "<img src=\"#{avatar_url}\" alt=\"Banana sample\" />"
 
       allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
-      avatar_url = "#{gitlab_host}/uploads/system/project/avatar/#{project.id}/banana_sample.gif"
+      avatar_url = "#{gitlab_host}/uploads/-/system/project/avatar/#{project.id}/banana_sample.gif"
 
       expect(helper.project_icon(project.full_path).to_s)
         .to eq "<img src=\"#{avatar_url}\" alt=\"Banana sample\" />"
@@ -88,7 +88,7 @@ describe ApplicationHelper do
       context 'when there is a matching user' do
         it 'returns a relative URL for the avatar' do
           expect(helper.avatar_icon(user.email).to_s)
-            .to eq("/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+            .to eq("/uploads/-/system/user/avatar/#{user.id}/banana_sample.gif")
         end
 
         context 'when an asset_host is set in the config' do
@@ -100,14 +100,14 @@ describe ApplicationHelper do
 
           it 'returns an absolute URL on that asset host' do
             expect(helper.avatar_icon(user.email, only_path: false).to_s)
-              .to eq("#{asset_host}/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+              .to eq("#{asset_host}/uploads/-/system/user/avatar/#{user.id}/banana_sample.gif")
           end
         end
 
         context 'when only_path is set to false' do
           it 'returns an absolute URL for the avatar' do
             expect(helper.avatar_icon(user.email, only_path: false).to_s)
-              .to eq("#{gitlab_host}/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+              .to eq("#{gitlab_host}/uploads/-/system/user/avatar/#{user.id}/banana_sample.gif")
           end
         end
 
@@ -120,7 +120,7 @@ describe ApplicationHelper do
 
           it 'returns a relative URL with the correct prefix' do
             expect(helper.avatar_icon(user.email).to_s)
-              .to eq("/gitlab/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+              .to eq("/gitlab/uploads/-/system/user/avatar/#{user.id}/banana_sample.gif")
           end
         end
       end
@@ -138,14 +138,14 @@ describe ApplicationHelper do
       context 'when only_path is true' do
         it 'returns a relative URL for the avatar' do
           expect(helper.avatar_icon(user, only_path: true).to_s)
-            .to eq("/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+            .to eq("/uploads/-/system/user/avatar/#{user.id}/banana_sample.gif")
         end
       end
 
       context 'when only_path is false' do
         it 'returns an absolute URL for the avatar' do
           expect(helper.avatar_icon(user, only_path: false).to_s)
-            .to eq("#{gitlab_host}/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+            .to eq("#{gitlab_host}/uploads/-/system/user/avatar/#{user.id}/banana_sample.gif")
         end
       end
     end
diff --git a/spec/helpers/emails_helper_spec.rb b/spec/helpers/emails_helper_spec.rb
index c68e4f56b05..2390c1f3e5d 100644
--- a/spec/helpers/emails_helper_spec.rb
+++ b/spec/helpers/emails_helper_spec.rb
@@ -52,7 +52,7 @@ describe EmailsHelper do
         )
 
         expect(header_logo).to eq(
-          %{<img style="height: 50px" src="/uploads/system/appearance/header_logo/#{appearance.id}/dk.png" alt="Dk" />}
+          %{<img style="height: 50px" src="/uploads/-/system/appearance/header_logo/#{appearance.id}/dk.png" alt="Dk" />}
         )
       end
     end
diff --git a/spec/helpers/groups_helper_spec.rb b/spec/helpers/groups_helper_spec.rb
index e3f9d9db9eb..3a246f10283 100644
--- a/spec/helpers/groups_helper_spec.rb
+++ b/spec/helpers/groups_helper_spec.rb
@@ -11,7 +11,7 @@ describe GroupsHelper do
       group.avatar = fixture_file_upload(avatar_file_path)
       group.save!
       expect(group_icon(group.path).to_s)
-        .to match("/uploads/system/group/avatar/#{group.id}/banana_sample.gif")
+        .to match("/uploads/-/system/group/avatar/#{group.id}/banana_sample.gif")
     end
 
     it 'gives default avatar_icon when no avatar is present' do
diff --git a/spec/helpers/page_layout_helper_spec.rb b/spec/helpers/page_layout_helper_spec.rb
index 95b4032616e..9aca3987657 100644
--- a/spec/helpers/page_layout_helper_spec.rb
+++ b/spec/helpers/page_layout_helper_spec.rb
@@ -60,7 +60,7 @@ describe PageLayoutHelper do
     %w(project user group).each do |type|
       context "with @#{type} assigned" do
         it "uses #{type.titlecase} avatar if available" do
-          object = double(avatar_url: 'http://example.com/uploads/system/avatar.png')
+          object = double(avatar_url: 'http://example.com/uploads/-/system/avatar.png')
           assign(type, object)
 
           expect(helper.page_image).to eq object.avatar_url
diff --git a/spec/javascripts/vue_shared/components/commit_spec.js b/spec/javascripts/vue_shared/components/commit_spec.js
index 1c3188cdda2..d5754aaa9e7 100644
--- a/spec/javascripts/vue_shared/components/commit_spec.js
+++ b/spec/javascripts/vue_shared/components/commit_spec.js
@@ -22,7 +22,7 @@ describe('Commit component', () => {
         shortSha: 'b7836edd',
         title: 'Commit message',
         author: {
-          avatar_url: 'https://gitlab.com/uploads/system/user/avatar/300478/avatar.png',
+          avatar_url: 'https://gitlab.com/uploads/-/system/user/avatar/300478/avatar.png',
           web_url: 'https://gitlab.com/jschatz1',
           path: '/jschatz1',
           username: 'jschatz1',
@@ -45,7 +45,7 @@ describe('Commit component', () => {
         shortSha: 'b7836edd',
         title: 'Commit message',
         author: {
-          avatar_url: 'https://gitlab.com/uploads/system/user/avatar/300478/avatar.png',
+          avatar_url: 'https://gitlab.com/uploads/-/system/user/avatar/300478/avatar.png',
           web_url: 'https://gitlab.com/jschatz1',
           path: '/jschatz1',
           username: 'jschatz1',
diff --git a/spec/lib/gitlab/background_migration/migrate_system_uploads_to_new_folder_spec.rb b/spec/lib/gitlab/background_migration/migrate_system_uploads_to_new_folder_spec.rb
new file mode 100644
index 00000000000..a910fb105a5
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/migrate_system_uploads_to_new_folder_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::MigrateSystemUploadsToNewFolder do
+  let(:migration) { described_class.new }
+
+  before do
+    allow(migration).to receive(:logger).and_return(Logger.new(nil))
+  end
+
+  describe '#perform' do
+    it 'renames the path of system-uploads', truncate: true do
+      upload = create(:upload, model: create(:empty_project), path: 'uploads/system/project/avatar.jpg')
+
+      migration.perform('uploads/system/', 'uploads/-/system/')
+
+      expect(upload.reload.path).to eq('uploads/-/system/project/avatar.jpg')
+    end
+  end
+end
diff --git a/spec/migrations/cleanup_move_system_upload_folder_symlink_spec.rb b/spec/migrations/cleanup_move_system_upload_folder_symlink_spec.rb
new file mode 100644
index 00000000000..3a9fa8c7113
--- /dev/null
+++ b/spec/migrations/cleanup_move_system_upload_folder_symlink_spec.rb
@@ -0,0 +1,35 @@
+require 'spec_helper'
+require Rails.root.join("db", "post_migrate", "20170717111152_cleanup_move_system_upload_folder_symlink.rb")
+
+describe CleanupMoveSystemUploadFolderSymlink do
+  let(:migration) { described_class.new }
+  let(:test_base) { File.join(Rails.root, 'tmp', 'tests', 'move-system-upload-folder') }
+  let(:test_folder) { File.join(test_base, '-', 'system') }
+
+  before do
+    allow(migration).to receive(:base_directory).and_return(test_base)
+    FileUtils.rm_rf(test_base)
+    FileUtils.mkdir_p(test_folder)
+    allow(migration).to receive(:say)
+  end
+
+  describe '#up' do
+    before do
+      FileUtils.ln_s(test_folder, File.join(test_base, 'system'))
+    end
+
+    it 'removes the symlink' do
+      migration.up
+
+      expect(File.exist?(File.join(test_base, 'system'))).to be_falsey
+    end
+  end
+
+  describe '#down' do
+    it 'creates the symlink' do
+      migration.down
+
+      expect(File.symlink?(File.join(test_base, 'system'))).to be_truthy
+    end
+  end
+end
diff --git a/spec/migrations/move_system_upload_folder_spec.rb b/spec/migrations/move_system_upload_folder_spec.rb
new file mode 100644
index 00000000000..b622b4e9536
--- /dev/null
+++ b/spec/migrations/move_system_upload_folder_spec.rb
@@ -0,0 +1,62 @@
+require 'spec_helper'
+require Rails.root.join("db", "migrate", "20170717074009_move_system_upload_folder.rb")
+
+describe MoveSystemUploadFolder do
+  let(:migration) { described_class.new }
+  let(:test_base) { File.join(Rails.root, 'tmp', 'tests', 'move-system-upload-folder') }
+
+  before do
+    allow(migration).to receive(:base_directory).and_return(test_base)
+    FileUtils.rm_rf(test_base)
+    FileUtils.mkdir_p(test_base)
+    allow(migration).to receive(:say)
+  end
+
+  describe '#up' do
+    let(:test_folder) { File.join(test_base, 'system') }
+    let(:test_file) { File.join(test_folder, 'file') }
+
+    before do
+      FileUtils.mkdir_p(test_folder)
+      FileUtils.touch(test_file)
+    end
+
+    it 'moves the related folder' do
+      migration.up
+
+      expect(File.exist?(File.join(test_base, '-', 'system', 'file'))).to be_truthy
+    end
+
+    it 'creates a symlink linking making the new folder available on the old path' do
+      migration.up
+
+      expect(File.symlink?(File.join(test_base, 'system'))).to be_truthy
+      expect(File.exist?(File.join(test_base, 'system', 'file'))).to be_truthy
+    end
+  end
+
+  describe '#down' do
+    let(:test_folder) { File.join(test_base, '-', 'system') }
+    let(:test_file) { File.join(test_folder, 'file') }
+
+    before do
+      FileUtils.mkdir_p(test_folder)
+      FileUtils.touch(test_file)
+    end
+
+    it 'moves the system folder back to the old location' do
+      migration.down
+
+      expect(File.exist?(File.join(test_base, 'system', 'file'))).to be_truthy
+    end
+
+    it 'removes the symlink if it existed' do
+      FileUtils.ln_s(test_folder, File.join(test_base, 'system'))
+
+      migration.down
+
+      expect(File.directory?(File.join(test_base, 'system'))).to be_truthy
+      expect(File.symlink?(File.join(test_base, 'system'))).to be_falsey
+    end
+  end
+end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 066d7b9307f..770176451fe 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -189,7 +189,7 @@ describe Group, models: true do
     let!(:group) { create(:group, :access_requestable, :with_avatar) }
     let(:user) { create(:user) }
     let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
-    let(:avatar_path) { "/uploads/system/group/avatar/#{group.id}/dk.png" }
+    let(:avatar_path) { "/uploads/-/system/group/avatar/#{group.id}/dk.png" }
 
     context 'when avatar file is uploaded' do
       before do
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 89ea5ceda95..a4090b37f65 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -44,7 +44,7 @@ describe Namespace, models: true do
       end
 
       context "is case insensitive" do
-        let(:group) { build(:group, path: "System") }
+        let(:group) { build(:group, path: "Groups") }
 
         it { expect(group).not_to be_valid }
       end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 74da86eaae6..aab99c28344 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -807,7 +807,7 @@ describe Project, models: true do
 
     context 'when avatar file is uploaded' do
       let(:project) { create(:empty_project, :with_avatar) }
-      let(:avatar_path) { "/uploads/system/project/avatar/#{project.id}/dk.png" }
+      let(:avatar_path) { "/uploads/-/system/project/avatar/#{project.id}/dk.png" }
       let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
 
       it 'shows correct url' do
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index e5a82b4d340..61670e71921 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1028,7 +1028,7 @@ describe User, models: true do
 
     context 'when avatar file is uploaded' do
       let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
-      let(:avatar_path) { "/uploads/system/user/avatar/#{user.id}/dk.png" }
+      let(:avatar_path) { "/uploads/-/system/user/avatar/#{user.id}/dk.png" }
 
       it 'shows correct avatar url' do
         expect(user.avatar_url).to eq(avatar_path)
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index fa704f23857..6dbde8bad31 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -442,7 +442,7 @@ describe API::Projects do
       post api('/projects', user), project
 
       project_id = json_response['id']
-      expect(json_response['avatar_url']).to eq("http://localhost/uploads/system/project/avatar/#{project_id}/banana_sample.gif")
+      expect(json_response['avatar_url']).to eq("http://localhost/uploads/-/system/project/avatar/#{project_id}/banana_sample.gif")
     end
 
     it 'sets a project as allowing merge even if build fails' do
diff --git a/spec/requests/openid_connect_spec.rb b/spec/requests/openid_connect_spec.rb
index 6d1f0b24196..2803cf8945a 100644
--- a/spec/requests/openid_connect_spec.rb
+++ b/spec/requests/openid_connect_spec.rb
@@ -79,7 +79,7 @@ describe 'OpenID Connect requests' do
           'email_verified' => true,
           'website'        => 'https://example.com',
           'profile'        => 'http://localhost/alice',
-          'picture'        => "http://localhost/uploads/system/user/avatar/#{user.id}/dk.png"
+          'picture'        => "http://localhost/uploads/-/system/user/avatar/#{user.id}/dk.png"
         })
       end
     end
diff --git a/spec/services/projects/participants_service_spec.rb b/spec/services/projects/participants_service_spec.rb
index d75851134ee..3688f6d4e23 100644
--- a/spec/services/projects/participants_service_spec.rb
+++ b/spec/services/projects/participants_service_spec.rb
@@ -13,7 +13,7 @@ describe Projects::ParticipantsService, services: true do
         groups = participants.groups
 
         expect(groups.size).to eq 1
-        expect(groups.first[:avatar_url]).to eq("/uploads/system/group/avatar/#{group.id}/dk.png")
+        expect(groups.first[:avatar_url]).to eq("/uploads/-/system/group/avatar/#{group.id}/dk.png")
       end
 
       it 'should return an url for the avatar with relative url' do
@@ -24,7 +24,7 @@ describe Projects::ParticipantsService, services: true do
         groups = participants.groups
 
         expect(groups.size).to eq 1
-        expect(groups.first[:avatar_url]).to eq("/gitlab/uploads/system/group/avatar/#{group.id}/dk.png")
+        expect(groups.first[:avatar_url]).to eq("/gitlab/uploads/-/system/group/avatar/#{group.id}/dk.png")
       end
     end
   end
diff --git a/spec/uploaders/attachment_uploader_spec.rb b/spec/uploaders/attachment_uploader_spec.rb
index d82dbe871d5..04ee6e9bfad 100644
--- a/spec/uploaders/attachment_uploader_spec.rb
+++ b/spec/uploaders/attachment_uploader_spec.rb
@@ -5,7 +5,7 @@ describe AttachmentUploader do
 
   describe "#store_dir" do
     it "stores in the system dir" do
-      expect(uploader.store_dir).to start_with("uploads/system/user")
+      expect(uploader.store_dir).to start_with("uploads/-/system/user")
     end
 
     it "uses the old path when using object storage" do
diff --git a/spec/uploaders/avatar_uploader_spec.rb b/spec/uploaders/avatar_uploader_spec.rb
index 201fe6949aa..1dc574699d8 100644
--- a/spec/uploaders/avatar_uploader_spec.rb
+++ b/spec/uploaders/avatar_uploader_spec.rb
@@ -5,7 +5,7 @@ describe AvatarUploader do
 
   describe "#store_dir" do
     it "stores in the system dir" do
-      expect(uploader.store_dir).to start_with("uploads/system/user")
+      expect(uploader.store_dir).to start_with("uploads/-/system/user")
     end
 
     it "uses the old path when using object storage" do