Merge branch 'storage-validation-improvement' into 'master'

Improve storage validation after configuration structure update

See merge request !9811

2 files changed, 63 insertions, 45 deletions

diff --git a/config/initializers/6_validations.rb b/config/initializers/6_validations.rb
index abe570f430c..9e24f42d284 100644
--- a/config/initializers/6_validations.rb
+++ b/config/initializers/6_validations.rb
@@ -13,24 +13,27 @@ def storage_validation_error(message)
   raise "#{message}. Please fix this in your gitlab.yml before starting GitLab."
 end
 
-def validate_storages
+def validate_storages_config
   storage_validation_error('No repository storage path defined') if Gitlab.config.repositories.storages.empty?
 
   Gitlab.config.repositories.storages.each do |name, repository_storage|
     storage_validation_error("\"#{name}\" is not a valid storage name") unless storage_name_valid?(name)
 
     if repository_storage.is_a?(String)
-      error = "#{name} is not a valid storage, because it has no `path` key. " \
+      raise "#{name} is not a valid storage, because it has no `path` key. " \
         "It may be configured as:\n\n#{name}:\n  path: #{repository_storage}\n\n" \
-        "Refer to gitlab.yml.example for an updated example"
-
-      storage_validation_error(error)
+        "For source installations, update your config/gitlab.yml Refer to gitlab.yml.example for an updated example.\n\n" \
+        "If you're using the Gitlab Development Kit, you can update your configuration running `gdk reconfigure`.\n"
     end
 
     if !repository_storage.is_a?(Hash) || repository_storage['path'].nil?
       storage_validation_error("#{name} is not a valid storage, because it has no `path` key. Refer to gitlab.yml.example for an updated example")
     end
+  end
+end
 
+def validate_storages_paths
+  Gitlab.config.repositories.storages.each do |name, repository_storage|
     parent_name, _parent_path = find_parent_path(name, repository_storage['path'])
     if parent_name
       storage_validation_error("#{name} is a nested path of #{parent_name}. Nested paths are not supported for repository storages")
@@ -38,4 +41,5 @@ def validate_storages
   end
 end
 
-validate_storages unless Rails.env.test? || ENV['SKIP_STORAGE_VALIDATION'] == 'true'
+validate_storages_config
+validate_storages_paths unless Rails.env.test? || ENV['SKIP_STORAGE_VALIDATION'] == 'true'
diff --git a/spec/initializers/6_validations_spec.rb b/spec/initializers/6_validations_spec.rb
index cf182e6d221..374517fec37 100644
--- a/spec/initializers/6_validations_spec.rb
+++ b/spec/initializers/6_validations_spec.rb
@@ -12,63 +12,77 @@ describe '6_validations', lib: true do
     FileUtils.rm_rf('tmp/tests/paths')
   end
 
-  context 'with correct settings' do
-    before do
-      mock_storages('foo' => { 'path' => 'tmp/tests/paths/a/b/c' }, 'bar' => { 'path' => 'tmp/tests/paths/a/b/d' })
-    end
+  describe 'validate_storages_config' do
+    context 'with correct settings' do
+      before do
+        mock_storages('foo' => { 'path' => 'tmp/tests/paths/a/b/c' }, 'bar' => { 'path' => 'tmp/tests/paths/a/b/d' })
+      end
 
-    it 'passes through' do
-      expect { validate_storages }.not_to raise_error
+      it 'passes through' do
+        expect { validate_storages_config }.not_to raise_error
+      end
     end
-  end
 
-  context 'with invalid storage names' do
-    before do
-      mock_storages('name with spaces' => { 'path' => 'tmp/tests/paths/a/b/c' })
-    end
+    context 'with invalid storage names' do
+      before do
+        mock_storages('name with spaces' => { 'path' => 'tmp/tests/paths/a/b/c' })
+      end
 
-    it 'throws an error' do
-      expect { validate_storages }.to raise_error('"name with spaces" is not a valid storage name. Please fix this in your gitlab.yml before starting GitLab.')
+      it 'throws an error' do
+        expect { validate_storages_config }.to raise_error('"name with spaces" is not a valid storage name. Please fix this in your gitlab.yml before starting GitLab.')
+      end
     end
-  end
 
-  context 'with nested storage paths' do
-    before do
-      mock_storages('foo' => { 'path' => 'tmp/tests/paths/a/b/c' }, 'bar' => { 'path' => 'tmp/tests/paths/a/b/c/d' })
-    end
+    context 'with incomplete settings' do
+      before do
+        mock_storages('foo' => {})
+      end
 
-    it 'throws an error' do
-      expect { validate_storages }.to raise_error('bar is a nested path of foo. Nested paths are not supported for repository storages. Please fix this in your gitlab.yml before starting GitLab.')
+      it 'throws an error suggesting the user to update its settings' do
+        expect { validate_storages_config }.to raise_error('foo is not a valid storage, because it has no `path` key. Refer to gitlab.yml.example for an updated example. Please fix this in your gitlab.yml before starting GitLab.')
+      end
     end
-  end
 
-  context 'with similar but un-nested storage paths' do
-    before do
-      mock_storages('foo' => { 'path' => 'tmp/tests/paths/a/b/c' }, 'bar' => { 'path' => 'tmp/tests/paths/a/b/c2' })
-    end
+    context 'with deprecated settings structure' do
+      before do
+        mock_storages('foo' => 'tmp/tests/paths/a/b/c')
+      end
 
-    it 'passes through' do
-      expect { validate_storages }.not_to raise_error
+      it 'throws an error suggesting the user to update its settings' do
+        expect { validate_storages_config }.to raise_error("foo is not a valid storage, because it has no `path` key. It may be configured as:\n\nfoo:\n  path: tmp/tests/paths/a/b/c\n\nFor source installations, update your config/gitlab.yml Refer to gitlab.yml.example for an updated example.\n\nIf you're using the Gitlab Development Kit, you can update your configuration running `gdk reconfigure`.\n")
+      end
     end
   end
 
-  context 'with incomplete settings' do
-    before do
-      mock_storages('foo' => {})
-    end
+  describe 'validate_storages_paths' do
+    context 'with correct settings' do
+      before do
+        mock_storages('foo' => { 'path' => 'tmp/tests/paths/a/b/c' }, 'bar' => { 'path' => 'tmp/tests/paths/a/b/d' })
+      end
 
-    it 'throws an error suggesting the user to update its settings' do
-      expect { validate_storages }.to raise_error('foo is not a valid storage, because it has no `path` key. Refer to gitlab.yml.example for an updated example. Please fix this in your gitlab.yml before starting GitLab.')
+      it 'passes through' do
+        expect { validate_storages_paths }.not_to raise_error
+      end
     end
-  end
 
-  context 'with deprecated settings structure' do
-    before do
-      mock_storages('foo' => 'tmp/tests/paths/a/b/c')
+    context 'with nested storage paths' do
+      before do
+        mock_storages('foo' => { 'path' => 'tmp/tests/paths/a/b/c' }, 'bar' => { 'path' => 'tmp/tests/paths/a/b/c/d' })
+      end
+
+      it 'throws an error' do
+        expect { validate_storages_paths }.to raise_error('bar is a nested path of foo. Nested paths are not supported for repository storages. Please fix this in your gitlab.yml before starting GitLab.')
+      end
     end
 
-    it 'throws an error suggesting the user to update its settings' do
-      expect { validate_storages }.to raise_error("foo is not a valid storage, because it has no `path` key. It may be configured as:\n\nfoo:\n  path: tmp/tests/paths/a/b/c\n\nRefer to gitlab.yml.example for an updated example. Please fix this in your gitlab.yml before starting GitLab.")
+    context 'with similar but un-nested storage paths' do
+      before do
+        mock_storages('foo' => { 'path' => 'tmp/tests/paths/a/b/c' }, 'bar' => { 'path' => 'tmp/tests/paths/a/b/c2' })
+      end
+
+      it 'passes through' do
+        expect { validate_storages_paths }.not_to raise_error
+      end
     end
   end