Fixed and improved some existing checks and SystemCheck library

12 files changed, 61 insertions, 77 deletions

diff --git a/lib/system_check.rb b/lib/system_check.rb
index e9cbf6b8258..466c39904fa 100644
--- a/lib/system_check.rb
+++ b/lib/system_check.rb
@@ -9,22 +9,13 @@ module SystemCheck
   #
   # @param [String] component name of the component relative to the checks being executed
   # @param [Array<BaseCheck>] checks classes of corresponding checks to be executed in the same order
-  # @param [BaseExecutor] executor_klass optionally specifiy a different executor class
-  def self.run(component, checks = [], executor_klass = SimpleExecutor)
-    unless executor_klass < BaseExecutor
-      raise ArgumentError, 'Invalid executor'
-    end
-
-    prepare(component, checks, executor_klass).execute
-  end
+  def self.run(component, checks = [])
+    executor = SimpleExecutor.new(component)
 
-  def self.prepare(component, checks = [], executor_klass = SimpleExecutor)
-    executor = executor_klass.new(component)
     checks.each do |check|
       executor << check
     end
 
-    executor
+    executor.execute
   end
-  private_class_method :prepare
 end
diff --git a/lib/system_check/app/database_config_exists_check.rb b/lib/system_check/app/database_config_exists_check.rb
index d557cee47b4..d1fae192350 100644
--- a/lib/system_check/app/database_config_exists_check.rb
+++ b/lib/system_check/app/database_config_exists_check.rb
@@ -15,17 +15,11 @@ module SystemCheck
           'Check that the information in config/database.yml is correct'
         )
         for_more_information(
-          see_database_guide,
+          'doc/install/databases.md',
           'http://guides.rubyonrails.org/getting_started.html#configuring-a-database'
         )
         fix_and_rerun
       end
-
-      private
-
-      def see_database_guide
-        'doc/install/databases.md'
-      end
     end
   end
 end
diff --git a/lib/system_check/app/git_config_check.rb b/lib/system_check/app/git_config_check.rb
index 7f0c792eb35..198867f7ac6 100644
--- a/lib/system_check/app/git_config_check.rb
+++ b/lib/system_check/app/git_config_check.rb
@@ -5,7 +5,7 @@ module SystemCheck
         'core.autocrlf' => 'input'
       }.freeze
 
-      set_name 'Git configured with autocrlf=input?'
+      set_name 'Git configured correctly?'
 
       def check?
         correct_options = OPTIONS.map do |name, value|
@@ -15,8 +15,18 @@ module SystemCheck
         correct_options.all?
       end
 
+      # Tries to configure git itself
+      #
+      # Returns true if all subcommands were successful (according to their exit code)
+      # Returns false if any or all subcommands failed.
       def repair!
-        auto_fix_git_config(OPTIONS)
+        return false unless is_gitlab_user?
+
+        command_success = OPTIONS.map do |name, value|
+          system(*%W(#{Gitlab.config.git.bin_path} config --global #{name} #{value}))
+        end
+
+        command_success.all?
       end
 
       def show_error
@@ -27,24 +37,6 @@ module SystemCheck
           see_installation_guide_section 'GitLab'
         )
       end
-
-      private
-
-      # Tries to configure git itself
-      #
-      # Returns true if all subcommands were successfull (according to their exit code)
-      # Returns false if any or all subcommands failed.
-      def auto_fix_git_config(options)
-        if !@warned_user_not_gitlab
-          command_success = options.map do |name, value|
-            system(*%W(#{Gitlab.config.git.bin_path} config --global #{name} #{value}))
-          end
-
-          command_success.all?
-        else
-          false
-        end
-      end
     end
   end
 end
diff --git a/lib/system_check/app/gitlab_config_not_outdated_check.rb b/lib/system_check/app/gitlab_config_up_to_date_check.rb
index 8a4d7b29977..c609e48e133 100644
--- a/lib/system_check/app/gitlab_config_not_outdated_check.rb
+++ b/lib/system_check/app/gitlab_config_up_to_date_check.rb
@@ -1,9 +1,7 @@
 module SystemCheck
   module App
-    class GitlabConfigNotOutdatedCheck < SystemCheck::BaseCheck
-      set_name 'GitLab config outdated?'
-      set_check_pass 'no'
-      set_check_fail 'yes'
+    class GitlabConfigUpToDateCheck < SystemCheck::BaseCheck
+      set_name 'GitLab config up to date?'
       set_skip_reason "can't check because of previous errors"
 
       def skip?
@@ -18,7 +16,7 @@ module SystemCheck
 
       def show_error
         try_fixing_it(
-          'Backup your config/gitlab.yml',
+          'Back-up your config/gitlab.yml',
           'Copy config/gitlab.yml.example to config/gitlab.yml',
           'Update config/gitlab.yml to match your setup'
         )
diff --git a/lib/system_check/app/projects_have_namespace_check.rb b/lib/system_check/app/projects_have_namespace_check.rb
index c70633a6d4f..a6ec9f7665c 100644
--- a/lib/system_check/app/projects_have_namespace_check.rb
+++ b/lib/system_check/app/projects_have_namespace_check.rb
@@ -1,7 +1,7 @@
 module SystemCheck
   module App
     class ProjectsHaveNamespaceCheck < SystemCheck::BaseCheck
-      set_name 'projects have namespace: '
+      set_name 'Projects have namespace:'
       set_skip_reason "can't check, you have no projects"
 
       def skip?
diff --git a/lib/system_check/app/ruby_version_check.rb b/lib/system_check/app/ruby_version_check.rb
index 37b4d24aa55..fd82f5f8a4a 100644
--- a/lib/system_check/app/ruby_version_check.rb
+++ b/lib/system_check/app/ruby_version_check.rb
@@ -5,7 +5,7 @@ module SystemCheck
       set_check_pass -> { "yes (#{self.current_version})" }
 
       def self.required_version
-        @required_version ||= Gitlab::VersionInfo.new(2, 1, 0)
+        @required_version ||= Gitlab::VersionInfo.new(2, 3, 3)
       end
 
       def self.current_version
diff --git a/lib/system_check/base_check.rb b/lib/system_check/base_check.rb
index 63b7eea5add..5dcb3f0886b 100644
--- a/lib/system_check/base_check.rb
+++ b/lib/system_check/base_check.rb
@@ -1,10 +1,7 @@
-require 'tasks/gitlab/task_helpers'
-
 module SystemCheck
   # Base class for Checks. You must inherit from here
   # and implement the methods below when necessary
   class BaseCheck
-    include ::Gitlab::TaskHelpers
     include ::SystemCheck::Helpers
 
     # Define a custom term for when check passed
diff --git a/lib/system_check/helpers.rb b/lib/system_check/helpers.rb
index 0a6be42a7d1..cd54baa494f 100644
--- a/lib/system_check/helpers.rb
+++ b/lib/system_check/helpers.rb
@@ -1,5 +1,9 @@
+require 'tasks/gitlab/task_helpers'
+
 module SystemCheck
   module Helpers
+    include ::Gitlab::TaskHelpers
+
     # Display a message telling to fix and rerun the checks
     def fix_and_rerun
       $stdout.puts '  Please fix the error above and rerun the checks.'.color(:red)
@@ -9,8 +13,6 @@ module SystemCheck
     #
     # @param [Array<String>] sources one or more references (documentation or links)
     def for_more_information(*sources)
-      sources = sources.shift if sources.first.is_a?(Array)
-
       $stdout.puts '  For more information see:'.color(:blue)
       sources.each do |source|
         $stdout.puts "  #{source}"
@@ -73,5 +75,12 @@ module SystemCheck
     def sudo_gitlab(command)
       "sudo -u #{gitlab_user} -H #{command}"
     end
+
+    def is_gitlab_user?
+      return @is_gitlab_user unless @is_gitlab_user.nil?
+
+      current_user = run_command(%w(whoami)).chomp
+      @is_gitlab_user = current_user == gitlab_user
+    end
   end
 end
diff --git a/lib/system_check/simple_executor.rb b/lib/system_check/simple_executor.rb
index ad2f549b3fb..c5403693f7a 100644
--- a/lib/system_check/simple_executor.rb
+++ b/lib/system_check/simple_executor.rb
@@ -18,32 +18,32 @@ module SystemCheck
 
     # Executes a single check
     #
-    # @param [SystemCheck::BaseCheck] check
-    def run_check(check)
-      $stdout.print "#{check.display_name} ... "
+    # @param [SystemCheck::BaseCheck] check_klass
+    def run_check(check_klass)
+      $stdout.print "#{check_klass.display_name} ... "
 
-      c = check.new
+      check = check_klass.new
 
       # When implements skip method, we run it first, and if true, skip the check
-      if c.can_skip? && c.skip?
-        $stdout.puts check.skip_reason.color(:magenta)
+      if check.can_skip? && check.skip?
+        $stdout.puts check_klass.skip_reason.color(:magenta)
         return
       end
-      
+
       # When implements a multi check, we don't control the output
-      if c.is_multi_check?
-        c.multi_check
+      if check.is_multi_check?
+        check.multi_check
         return
       end
 
-      if c.check?
-        $stdout.puts check.check_pass.color(:green)
+      if check.check?
+        $stdout.puts check_klass.check_pass.color(:green)
       else
-        $stdout.puts check.check_fail.color(:red)
+        $stdout.puts check_klass.check_fail.color(:red)
 
-        if c.can_repair?
+        if check.can_repair?
           $stdout.print 'Trying to fix error automatically. ...'
-          if c.repair!
+          if check.repair!
             $stdout.puts 'Success'.color(:green)
             return
           else
@@ -51,7 +51,7 @@ module SystemCheck
           end
         end
 
-        c.show_error
+        check.show_error
       end
     end
 
diff --git a/lib/tasks/gitlab/check.rake b/lib/tasks/gitlab/check.rake
index 973517c99dd..63c5e9b9c83 100644
--- a/lib/tasks/gitlab/check.rake
+++ b/lib/tasks/gitlab/check.rake
@@ -21,7 +21,7 @@ namespace :gitlab do
         SystemCheck::App::MigrationsAreUpCheck,
         SystemCheck::App::OrphanedGroupMembersCheck,
         SystemCheck::App::GitlabConfigExistsCheck,
-        SystemCheck::App::GitlabConfigNotOutdatedCheck,
+        SystemCheck::App::GitlabConfigUpToDateCheck,
         SystemCheck::App::LogWritableCheck,
         SystemCheck::App::TmpWritableCheck,
         SystemCheck::App::UploadsDirectoryExistsCheck,
diff --git a/lib/tasks/gitlab/task_helpers.rb b/lib/tasks/gitlab/task_helpers.rb
index e3c9d3b491c..e2307e41be1 100644
--- a/lib/tasks/gitlab/task_helpers.rb
+++ b/lib/tasks/gitlab/task_helpers.rb
@@ -99,16 +99,17 @@ module Gitlab
     end
 
     def warn_user_is_not_gitlab
-      unless @warned_user_not_gitlab
-        gitlab_user = Gitlab.config.gitlab.user
+      return if @warned_user_not_gitlab
+
+      unless is_gitlab_user?
         current_user = run_command(%w(whoami)).chomp
-        unless current_user == gitlab_user
-          puts " Warning ".color(:black).background(:yellow)
-          puts "  You are running as user #{current_user.color(:magenta)}, we hope you know what you are doing."
-          puts "  Things may work\/fail for the wrong reasons."
-          puts "  For correct results you should run this as user #{gitlab_user.color(:magenta)}."
-          puts ""
-        end
+
+        puts " Warning ".color(:black).background(:yellow)
+        puts "  You are running as user #{current_user.color(:magenta)}, we hope you know what you are doing."
+        puts "  Things may work\/fail for the wrong reasons."
+        puts "  For correct results you should run this as user #{gitlab_user.color(:magenta)}."
+        puts ""
+
         @warned_user_not_gitlab = true
       end
     end
diff --git a/spec/lib/system_check/base_executor_spec.rb b/spec/lib/system_check/base_executor_spec.rb
index 139f7f3248f..4b379392da0 100644
--- a/spec/lib/system_check/base_executor_spec.rb
+++ b/spec/lib/system_check/base_executor_spec.rb
@@ -26,7 +26,7 @@ describe SystemCheck::BaseExecutor, lib: true do
       subject << SimpleCheck
     end
 
-    it 'returns an array of classes' do
+    it 'returns a set of classes' do
       expect(subject.checks).to include(SimpleCheck)
     end
   end
@@ -39,12 +39,14 @@ describe SystemCheck::BaseExecutor, lib: true do
     it 'appends a new check to the Set' do
       subject << OtherCheck
       stored_checks = subject.checks.to_a
+
       expect(stored_checks.first).to eq(SimpleCheck)
       expect(stored_checks.last).to eq(OtherCheck)
     end
 
     it 'inserts unique itens only' do
       subject << SimpleCheck
+
       expect(subject.checks.size).to eq(1)
     end
   end