Fixed specs for Gitlab::Redis and code for Redis Sentinel support

config/mail_room.yml

@@ -3,7 +3,7 @@
     require_relative "lib/gitlab/mail_room" unless defined?(Gitlab::MailRoom)
     config = Gitlab::MailRoom.config
 
-    if Gitlab::Mailroom.enabled?
+    if Gitlab::MailRoom.enabled?
   %>
     -
       :host: <%= config[:host].to_json %>

lib/gitlab/mail_room.rb

 require 'yaml'
 require 'json'
-require_relative 'lib/gitlab/redis' unless defined?(Gitlab::Redis)
+require_relative 'redis' unless defined?(Gitlab::Redis)
 
 module Gitlab
   module MailRoom
 
     class << self
-
       def enabled?
         config[:enabled] && config[:address]
       end
@@ -18,7 +17,7 @@ module Gitlab
       private
 
       def fetch_config
-        return nil unless File.exists?(config_file)
+        return {} unless File.exist?(config_file)
 
         rails_env = ENV['RAILS_ENV'] || ENV['RACK_ENV'] || 'development'
         all_config = YAML.load_file(config_file)[rails_env].deep_symbolize_keys
@@ -33,11 +32,12 @@ module Gitlab
         if config[:enabled] && config[:address]
           config[:redis_url] = Gitlab::Redis.new(rails_env).url
         end
+
+        config
       end
 
       def config_file
-        file = ENV['MAIL_ROOM_GITLAB_CONFIG_FILE'] || 'config/gitlab.yml'
-        File.expand_path("../../../#{file}", __FILE__)
+        ENV['MAIL_ROOM_GITLAB_CONFIG_FILE'] || File.expand_path('../../../config/gitlab.yml', __FILE__)
       end
     end
   end

lib/gitlab/redis.rb

@@ -19,7 +19,7 @@ module Gitlab
 
     class << self
       def params
-        @params || PARAMS_MUTEX.synchronize { @params = new.params }
+        PARAMS_MUTEX.synchronize { new.params }
       end
 
       # @deprecated Use .params instead to get sentinel support
@@ -38,7 +38,7 @@ module Gitlab
     end
 
     def initialize(rails_env=nil)
-      @rails_env = rails_env || Rails.env
+      @rails_env = rails_env || ::Rails.env
     end
 
     def params
@@ -55,7 +55,7 @@ module Gitlab
         # Redis::Store does not handle Unix sockets well, so let's do it for them
         config[:path] = redis_uri.path
       else
-        redis_hash = ::Redis::Store::Factory.extract_host_options_from_uri(redis_uri)
+        redis_hash = ::Redis::Store::Factory.extract_host_options_from_uri(config[:url])
         config.merge!(redis_hash)
       end
 
@@ -74,7 +74,8 @@ module Gitlab
     end
 
     def fetch_config
-      File.exists?(config_file) ? YAML.load_file(config_file)[@rails_env] : false
+      file = config_file
+      File.exist?(file) ? YAML.load_file(file)[@rails_env] : false
     end
 
     def config_file

spec/lib/gitlab/redis_config_spec.rb → spec/lib/gitlab/redis_spec.rb

 require 'spec_helper'
 
-describe Gitlab::RedisConfig do
-  let(:redis_config) { Rails.root.join('config', 'resque.yml') }
+describe Gitlab::Redis do
+  let(:redis_config) { Rails.root.join('config', 'resque.yml').to_s }
 
   describe '.params' do
     subject { described_class.params }
 
     context 'when url contains unix socket reference' do
-      let(:config_old) { Rails.root.join('spec/fixtures/config/redis_old_format_socket.yml') }
-      let(:config_new) { Rails.root.join('spec/fixtures/config/redis_new_format_socket.yml') }
+      let(:config_old) { Rails.root.join('spec/fixtures/config/redis_old_format_socket.yml').to_s }
+      let(:config_new) { Rails.root.join('spec/fixtures/config/redis_new_format_socket.yml').to_s }
 
       context 'with old format' do
         it 'returns path key instead' do
-          allow(Gitlab::RedisConfig).to receive(:config_file) { config_old }
+          expect_any_instance_of(described_class).to receive(:config_file) { config_old }
 
           is_expected.to include(path: '/path/to/redis.sock')
           is_expected.not_to have_key(:url)
@@ -21,7 +21,7 @@ describe Gitlab::RedisConfig do
 
       context 'with new format' do
         it 'returns path key instead' do
-          allow(Gitlab::RedisConfig).to receive(:config_file) { config_new }
+          expect_any_instance_of(described_class).to receive(:config_file) { config_new }
 
           is_expected.to include(path: '/path/to/redis.sock')
           is_expected.not_to have_key(:url)
@@ -35,7 +35,7 @@ describe Gitlab::RedisConfig do
 
       context 'with old format' do
         it 'returns hash with host, port, db, and password' do
-          allow(Gitlab::RedisConfig).to receive(:config_file) { config_old }
+          expect_any_instance_of(described_class).to receive(:config_file) { config_old }
 
           is_expected.to include(host: 'localhost', password: 'mypassword', port: 6379, db: 99)
           is_expected.not_to have_key(:url)
@@ -44,7 +44,7 @@ describe Gitlab::RedisConfig do
 
       context 'with new format' do
         it 'returns hash with host, port, db, and password' do
-          allow(Gitlab::RedisConfig).to receive(:config_file) { config_new }
+          expect_any_instance_of(described_class).to receive(:config_file) { config_new }
 
           is_expected.to include(host: 'localhost', password: 'mypassword', port: 6379, db: 99)
           is_expected.not_to have_key(:url)
@@ -53,29 +53,25 @@ describe Gitlab::RedisConfig do
     end
   end
 
-  describe '.raw_params' do
-    subject { described_class.send(:raw_params) }
-
+  describe '#raw_config_hash' do
     it 'returns default redis url when no config file is present' do
-      expect(Gitlab::RedisConfig).to receive(:fetch_config) { false }
+      expect(subject).to receive(:fetch_config) { false }
 
-      is_expected.to eq(url: Gitlab::RedisConfig::DEFAULT_REDIS_URL)
+      expect(subject.send(:raw_config_hash)).to eq(url: Gitlab::Redis::DEFAULT_REDIS_URL)
     end
 
     it 'returns old-style single url config in a hash' do
-      expect(Gitlab::RedisConfig).to receive(:fetch_config) { 'redis://myredis:6379' }
-      is_expected.to eq(url: 'redis://myredis:6379')
+      expect(subject).to receive(:fetch_config) { 'redis://myredis:6379' }
+      expect(subject.send(:raw_config_hash)).to eq(url: 'redis://myredis:6379')
     end
 
   end
 
-  describe '.fetch_config' do
-    subject { described_class.send(:fetch_config) }
-
+  describe '#fetch_config' do
     it 'returns false when no config file is present' do
-      allow(File).to receive(:exists?).with(redis_config) { false }
+      allow(File).to receive(:exist?).with(redis_config) { false }
 
-      is_expected.to be_falsey
+      expect(subject.send(:fetch_config)).to be_falsey
     end
   end
 end