Commit fca7a77a authored by Sean McGivern's avatar Sean McGivern

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

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

Closes #33550

See merge request !12917
parents 045e4de4 6f26f6f7
......@@ -195,7 +195,7 @@ module ProjectsHelper
controller.controller_name,
controller.action_name,
current_application_settings.cache_key,
'v2.4'
'v2.5'
]
key << pipeline_status_cache_key(project.pipeline_status) if project.pipeline_status.has_status?
......
......@@ -16,7 +16,7 @@ class GitlabUploader < CarrierWave::Uploader::Base
def self.base_dir
return root_dir unless file_storage?
File.join(root_dir, 'system')
File.join(root_dir, '-', 'system')
end
def self.file_storage?
......
---
title: "Move uploads from `uploads/system` to `uploads/-/system` to free up `system` as a group name"
merge_request: 11713
author:
scope path: :uploads do
# Note attachments and User/Group/Project avatars
get "system/:model/:mounted_as/:id/:filename",
get "-/system/:model/:mounted_as/:id/:filename",
to: "uploads#show",
constraints: { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /[^\/]+/ }
......@@ -15,7 +15,7 @@ scope path: :uploads do
constraints: { filename: /[^\/]+/ }
# Appearance
get "system/:model/:mounted_as/:id/:filename",
get "-/system/:model/:mounted_as/:id/:filename",
to: "uploads#show",
constraints: { model: /appearance/, mounted_as: /logo|header_logo/, filename: /.+/ }
......
class MoveSystemUploadFolder < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
DOWNTIME = false
def up
unless file_storage?
say 'Using object storage, no need to move.'
return
end
unless File.directory?(old_directory)
say "#{old_directory} doesn't exist, no need to move it."
return
end
FileUtils.mkdir_p(File.join(base_directory, '-'))
say "Moving #{old_directory} -> #{new_directory}"
FileUtils.mv(old_directory, new_directory)
FileUtils.ln_s(new_directory, old_directory)
end
def down
unless file_storage?
say 'Using object storage, no need to move.'
return
end
unless File.directory?(new_directory)
say "#{new_directory} doesn't exist, no need to move it."
return
end
if File.symlink?(old_directory)
say "Removing #{old_directory} -> #{new_directory} symlink"
FileUtils.rm(old_directory)
end
say "Moving #{new_directory} -> #{old_directory}"
FileUtils.mv(new_directory, old_directory)
end
def new_directory
File.join(base_directory, '-', 'system')
end
def old_directory
File.join(base_directory, 'system')
end
def base_directory
File.join(Rails.root, 'public', 'uploads')
end
def file_storage?
CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class CleanupMoveSystemUploadFolderSymlink < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
if File.symlink?(old_directory)
say "Removing #{old_directory} -> #{new_directory} symlink"
FileUtils.rm(old_directory)
else
say "Symlink #{old_directory} non existant, nothing to do."
end
end
def down
if File.directory?(new_directory)
say "Symlinking #{old_directory} -> #{new_directory}"
FileUtils.ln_s(new_directory, old_directory)
else
say "#{new_directory} doesn't exist, skipping."
end
end
def new_directory
File.join(base_directory, '-', 'system')
end
def old_directory
File.join(base_directory, 'system')
end
def base_directory
File.join(Rails.root, 'public', 'uploads')
end
end
class EnqueueMigrateSystemUploadsToNewFolder < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
OLD_FOLDER = 'uploads/system/'
NEW_FOLDER = 'uploads/-/system/'
disable_ddl_transaction!
def up
BackgroundMigrationWorker.perform_async('MigrateSystemUploadsToNewFolder',
[OLD_FOLDER, NEW_FOLDER])
end
def down
BackgroundMigrationWorker.perform_async('MigrateSystemUploadsToNewFolder',
[NEW_FOLDER, OLD_FOLDER])
end
end
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20170713104829) do
ActiveRecord::Schema.define(version: 20170717150329) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......
......@@ -81,7 +81,7 @@ class Spinach::Features::Groups < Spinach::FeatureSteps
step 'I should see new group "Owned" avatar' do
expect(owned_group.avatar).to be_instance_of AvatarUploader
expect(owned_group.avatar.url).to eq "/uploads/system/group/avatar/#{Group.find_by(name: "Owned").id}/banana_sample.gif"
expect(owned_group.avatar.url).to eq "/uploads/-/system/group/avatar/#{Group.find_by(name: "Owned").id}/banana_sample.gif"
end
step 'I should see the "Remove avatar" button' do
......
......@@ -36,7 +36,7 @@ class Spinach::Features::Profile < Spinach::FeatureSteps
step 'I should see new avatar' do
expect(@user.avatar).to be_instance_of AvatarUploader
expect(@user.avatar.url).to eq "/uploads/system/user/avatar/#{@user.id}/banana_sample.gif"
expect(@user.avatar.url).to eq "/uploads/-/system/user/avatar/#{@user.id}/banana_sample.gif"
end
step 'I should see the "Remove avatar" button' do
......
......@@ -38,7 +38,7 @@ class Spinach::Features::Project < Spinach::FeatureSteps
step 'I should see new project avatar' do
expect(@project.avatar).to be_instance_of AvatarUploader
url = @project.avatar.url
expect(url).to eq "/uploads/system/project/avatar/#{@project.id}/banana_sample.gif"
expect(url).to eq "/uploads/-/system/project/avatar/#{@project.id}/banana_sample.gif"
end
step 'I should see the "Remove avatar" button' do
......
module Gitlab
module BackgroundMigration
class MigrateSystemUploadsToNewFolder
include Gitlab::Database::MigrationHelpers
attr_reader :old_folder, :new_folder
class Upload < ActiveRecord::Base
self.table_name = 'uploads'
include EachBatch
end
def perform(old_folder, new_folder)
replace_sql = replace_sql(uploads[:path], old_folder, new_folder)
affected_uploads = Upload.where(uploads[:path].matches("#{old_folder}%"))
affected_uploads.each_batch do |batch|
batch.update_all("path = #{replace_sql}")
end
end
def uploads
Arel::Table.new('uploads')
end
end
end
end
......@@ -49,7 +49,6 @@ module Gitlab
sent_notifications
services
snippets
system
teams
u
unicorn_test
......
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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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',
......
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
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
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
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment