Merge branch 'system-checks-incoming-email' into 'master'

Move Incoming Email checks to System Checks

See merge request !14028

lib/system_check/app/init_script_up_to_date_check.rb

@@ -7,26 +7,22 @@ module SystemCheck
       set_skip_reason 'skipped (omnibus-gitlab has no init script)'
 
       def skip?
-        omnibus_gitlab?
-      end
+        return true if omnibus_gitlab?
 
-      def multi_check
-        recipe_path = Rails.root.join('lib/support/init.d/', 'gitlab')
+        unless init_file_exists?
+          self.skip_reason = "can't check because of previous errors"
 
-        unless File.exist?(SCRIPT_PATH)
-          $stdout.puts "can't check because of previous errors".color(:magenta)
-          return
+          true
+        end
       end
 
+      def check?
+        recipe_path = Rails.root.join('lib/support/init.d/', 'gitlab')
+
         recipe_content = File.read(recipe_path)
         script_content = File.read(SCRIPT_PATH)
 
-        if recipe_content == script_content
-          $stdout.puts 'yes'.color(:green)
-        else
-          $stdout.puts 'no'.color(:red)
-          show_error
-        end
+        recipe_content == script_content
       end
 
       def show_error
@@ -38,6 +34,12 @@ module SystemCheck
         )
         fix_and_rerun
       end
+
+      private
+
+      def init_file_exists?
+        File.exist?(SCRIPT_PATH)
+      end
     end
   end
 end

lib/system_check/base_check.rb

@@ -62,6 +62,25 @@ module SystemCheck
       call_or_return(@skip_reason) || 'skipped'
     end
 
+    # Define a reason why we skipped the SystemCheck (during runtime)
+    #
+    # This is used when you need dynamic evaluation like when you have
+    # multiple reasons why a check can fail
+    #
+    # @param [String] reason to be displayed
+    def skip_reason=(reason)
+      @skip_reason = reason
+    end
+
+    # Skip reason defined during runtime
+    #
+    # This value have precedence over the one defined in the subclass
+    #
+    # @return [String] the reason
+    def skip_reason
+      @skip_reason
+    end
+
     # Does the check support automatically repair routine?
     #
     # @return [Boolean] whether check implemented `#repair!` method or not

lib/system_check/incoming_email/foreman_configured_check.rb

+module SystemCheck
+  module IncomingEmail
+    class ForemanConfiguredCheck < SystemCheck::BaseCheck
+      set_name 'Foreman configured correctly?'
+
+      def check?
+        path = Rails.root.join('Procfile')
+
+        File.exist?(path) && File.read(path) =~ /^mail_room:/
+      end
+
+      def show_error
+        try_fixing_it(
+          'Enable mail_room in your Procfile.'
+        )
+        for_more_information(
+          'doc/administration/reply_by_email.md'
+        )
+        fix_and_rerun
+      end
+    end
+  end
+end

lib/system_check/incoming_email/imap_authentication_check.rb

+module SystemCheck
+  module IncomingEmail
+    class ImapAuthenticationCheck < SystemCheck::BaseCheck
+      set_name 'IMAP server credentials are correct?'
+
+      def check?
+        if mailbox_config
+          begin
+            imap = Net::IMAP.new(config[:host], port: config[:port], ssl: config[:ssl])
+            imap.starttls if config[:start_tls]
+            imap.login(config[:email], config[:password])
+            connected = true
+          rescue
+            connected = false
+          end
+        end
+
+        connected
+      end
+
+      def show_error
+        try_fixing_it(
+          'Check that the information in config/gitlab.yml is correct'
+        )
+        for_more_information(
+          'doc/administration/reply_by_email.md'
+        )
+        fix_and_rerun
+      end
+
+      private
+
+      def mailbox_config
+        return @config if @config
+
+        config_path = Rails.root.join('config', 'mail_room.yml').to_s
+        erb = ERB.new(File.read(config_path))
+        erb.filename = config_path
+        config_file = YAML.load(erb.result)
+
+        @config = config_file[:mailboxes]&.first
+      end
+    end
+  end
+end

lib/system_check/incoming_email/initd_configured_check.rb

+module SystemCheck
+  module IncomingEmail
+    class InitdConfiguredCheck < SystemCheck::BaseCheck
+      set_name 'Init.d configured correctly?'
+
+      def skip?
+        omnibus_gitlab?
+      end
+
+      def check?
+        mail_room_configured?
+      end
+
+      def show_error
+        try_fixing_it(
+          'Enable mail_room in the init.d configuration.'
+        )
+        for_more_information(
+          'doc/administration/reply_by_email.md'
+        )
+        fix_and_rerun
+      end
+
+      private
+
+      def mail_room_configured?
+        path = '/etc/default/gitlab'
+        File.exist?(path) && File.read(path).include?('mail_room_enabled=true')
+      end
+    end
+  end
+end

lib/system_check/incoming_email/mail_room_running_check.rb

+module SystemCheck
+  module IncomingEmail
+    class MailRoomRunningCheck < SystemCheck::BaseCheck
+      set_name 'MailRoom running?'
+
+      def skip?
+        return true if omnibus_gitlab?
+
+        unless mail_room_configured?
+          self.skip_reason = "can't check because of previous errors"
+          true
+        end
+      end
+
+      def check?
+        mail_room_running?
+      end
+
+      def show_error
+        try_fixing_it(
+          sudo_gitlab('RAILS_ENV=production bin/mail_room start')
+        )
+        for_more_information(
+          see_installation_guide_section('Install Init Script'),
+          'see log/mail_room.log for possible errors'
+        )
+        fix_and_rerun
+      end
+
+      private
+
+      def mail_room_configured?
+        path = '/etc/default/gitlab'
+        File.exist?(path) && File.read(path).include?('mail_room_enabled=true')
+      end
+
+      def mail_room_running?
+        ps_ux, _ = Gitlab::Popen.popen(%w(ps uxww))
+        ps_ux.include?("mail_room")
+      end
+    end
+  end
+end

lib/system_check/simple_executor.rb

@@ -23,7 +23,7 @@ module SystemCheck
     #
     # @param [BaseCheck] check class
     def <<(check)
-      raise ArgumentError unless check < BaseCheck
+      raise ArgumentError unless check.is_a?(Class) && check < BaseCheck
       @checks << check
     end
 
@@ -48,7 +48,7 @@ module SystemCheck
 
       # When implements skip method, we run it first, and if true, skip the check
       if check.can_skip? && check.skip?
-        $stdout.puts check_klass.skip_reason.color(:magenta)
+        $stdout.puts check.skip_reason.try(:color, :magenta) || check_klass.skip_reason.color(:magenta)
         return
       end
 

lib/tasks/gitlab/check.rake

@@ -309,132 +309,23 @@ namespace :gitlab do
     desc "GitLab | Check the configuration of Reply by email"
     task check: :environment  do
       warn_user_is_not_gitlab
-      start_checking "Reply by email"
 
       if Gitlab.config.incoming_email.enabled
-        check_imap_authentication
+        checks = [
+          SystemCheck::IncomingEmail::ImapAuthenticationCheck
+        ]
 
         if Rails.env.production?
-          check_initd_configured_correctly
-          check_mail_room_running
-        else
-          check_foreman_configured_correctly
-        end
-      else
-        puts 'Reply by email is disabled in config/gitlab.yml'
-      end
-
-      finished_checking "Reply by email"
-    end
-
-    # Checks
-    ########################
-
-    def check_initd_configured_correctly
-      return if omnibus_gitlab?
-
-      print "Init.d configured correctly? ... "
-
-      path = "/etc/default/gitlab"
-
-      if File.exist?(path) && File.read(path).include?("mail_room_enabled=true")
-        puts "yes".color(:green)
-      else
-        puts "no".color(:red)
-        try_fixing_it(
-          "Enable mail_room in the init.d configuration."
-        )
-        for_more_information(
-          "doc/administration/reply_by_email.md"
-        )
-        fix_and_rerun
-      end
-    end
-
-    def check_foreman_configured_correctly
-      print "Foreman configured correctly? ... "
-
-      path = Rails.root.join("Procfile")
-
-      if File.exist?(path) && File.read(path) =~ /^mail_room:/
-        puts "yes".color(:green)
-      else
-        puts "no".color(:red)
-        try_fixing_it(
-          "Enable mail_room in your Procfile."
-        )
-        for_more_information(
-          "doc/administration/reply_by_email.md"
-        )
-        fix_and_rerun
-      end
-    end
-
-    def check_mail_room_running
-      return if omnibus_gitlab?
-
-      print "MailRoom running? ... "
-
-      path = "/etc/default/gitlab"
-
-      unless File.exist?(path) && File.read(path).include?("mail_room_enabled=true")
-        puts "can't check because of previous errors".color(:magenta)
-        return
-      end
-
-      if mail_room_running?
-        puts "yes".color(:green)
+          checks << SystemCheck::IncomingEmail::InitdConfiguredCheck
+          checks << SystemCheck::IncomingEmail::MailRoomRunningCheck
         else
-        puts "no".color(:red)
-        try_fixing_it(
-          sudo_gitlab("RAILS_ENV=production bin/mail_room start")
-        )
-        for_more_information(
-          see_installation_guide_section("Install Init Script"),
-          "see log/mail_room.log for possible errors"
-        )
-        fix_and_rerun
-      end
+          checks << SystemCheck::IncomingEmail::ForemanConfiguredCheck
         end
 
-    def check_imap_authentication
-      print "IMAP server credentials are correct? ... "
-
-      config_path = Rails.root.join('config', 'mail_room.yml').to_s
-      erb = ERB.new(File.read(config_path))
-      erb.filename = config_path
-      config_file = YAML.load(erb.result)
-
-      config = config_file[:mailboxes].first
-
-      if config
-        begin
-          imap = Net::IMAP.new(config[:host], port: config[:port], ssl: config[:ssl])
-          imap.starttls if config[:start_tls]
-          imap.login(config[:email], config[:password])
-          connected = true
-        rescue
-          connected = false
-        end
-      end
-
-      if connected
-        puts "yes".color(:green)
+        SystemCheck.run('Reply by email', checks)
       else
-        puts "no".color(:red)
-        try_fixing_it(
-          "Check that the information in config/gitlab.yml is correct"
-        )
-        for_more_information(
-          "doc/administration/reply_by_email.md"
-        )
-        fix_and_rerun
-      end
+        puts 'Reply by email is disabled in config/gitlab.yml'
       end
-
-    def mail_room_running?
-      ps_ux, _ = Gitlab::Popen.popen(%w(ps uxww))
-      ps_ux.include?("mail_room")
     end
   end
 

spec/lib/system_check/simple_executor_spec.rb

@@ -35,6 +35,20 @@ describe SystemCheck::SimpleExecutor do
     end
   end
 
+  class DynamicSkipCheck < SystemCheck::BaseCheck
+    set_name 'dynamic skip check'
+    set_skip_reason 'this is a skip reason'
+
+    def skip?
+      self.skip_reason = 'this is a dynamic skip reason'
+      true
+    end
+
+    def check?
+      raise 'should not execute this'
+    end
+  end
+
   class MultiCheck < SystemCheck::BaseCheck
     set_name 'multi check'
 
@@ -127,6 +141,10 @@ describe SystemCheck::SimpleExecutor do
 
       expect(subject.checks.size).to eq(1)
     end
+
+    it 'errors out when passing multiple items' do
+      expect { subject << [SimpleCheck, OtherCheck] }.to raise_error(ArgumentError)
+    end
   end
 
   subject { described_class.new('Test') }
@@ -205,10 +223,14 @@ describe SystemCheck::SimpleExecutor do
         subject.run_check(SkipCheck)
       end
 
-      it 'displays #skip_reason' do
+      it 'displays .skip_reason' do
         expect { subject.run_check(SkipCheck) }.to output(/this is a skip reason/).to_stdout
       end
 
+      it 'displays #skip_reason' do
+        expect { subject.run_check(DynamicSkipCheck) }.to output(/this is a dynamic skip reason/).to_stdout
+      end
+
       it 'does not execute #check when #skip? is true' do
         expect_any_instance_of(SkipCheck).not_to receive(:check?)