diff options
author | Stan Hu <stanhu@gmail.com> | 2016-05-09 00:21:10 -0700 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2016-05-12 07:00:19 -0500 |
commit | 7f7359b1381d376b81fd9b81120d8cfa0231a526 (patch) | |
tree | bec8110ed8c14fb0dc38ced1ebf21177dbc2acc5 | |
parent | f0f1bbec8ad5d5fade7c0efeee22ba9b9bc44f07 (diff) | |
download | gitlab-shell-7f7359b1381d376b81fd9b81120d8cfa0231a526.tar.gz |
Use Redis Ruby client instead of shelling out to redis-cli
Closes gitlab-org/gitlab-ce#17329
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | Gemfile | 2 | ||||
-rw-r--r-- | Gemfile.lock | 5 | ||||
-rwxr-xr-x | bin/check | 5 | ||||
-rw-r--r-- | lib/gitlab_config.rb | 22 | ||||
-rw-r--r-- | lib/gitlab_net.rb | 19 | ||||
-rw-r--r-- | lib/gitlab_post_receive.rb | 10 | ||||
-rw-r--r-- | spec/gitlab_config_spec.rb | 38 | ||||
-rw-r--r-- | spec/gitlab_net_spec.rb | 40 | ||||
-rw-r--r-- | spec/gitlab_post_receive_spec.rb | 22 |
10 files changed, 85 insertions, 79 deletions
@@ -3,6 +3,7 @@ v3.0.0 - Remove create-branch and rm-branch commands (Robert Schilling) - Update PostReceive worker so it logs a unique JID in Sidekiq - Remove update-head command + - Use Redis Ruby client instead of shelling out to redis-cli v2.7.2 - Do not prune objects during 'git gc' @@ -1,5 +1,7 @@ source "http://rubygems.org" +gem 'redis', '~> 3.3' + group :development, :test do gem 'coveralls', require: false gem 'rspec', '~> 2.14.0' diff --git a/Gemfile.lock b/Gemfile.lock index 9bf7d9f..cd07c9b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -37,6 +37,7 @@ GEM method_source (~> 0.8) slop (~> 3.3.1) rainbow (2.0.0) + redis (3.3.0) rest-client (1.7.2) mime-types (>= 1.16, < 3.0) netrc (~> 0.7) @@ -77,7 +78,11 @@ DEPENDENCIES coveralls guard guard-rspec + redis (~> 3.3) rspec (~> 2.14.0) rubocop (= 0.28.0) vcr webmock + +BUNDLED WITH + 1.11.2 @@ -36,8 +36,5 @@ dirs.each do |dir| puts "\n" end -print "Test redis-cli executable: " -abort('FAILED') unless system(*config.redis_command, '--version') - print "Send ping to redis server: " -abort unless system(*config.redis_command, 'ping') +abort unless GitlabNet.new.redis_client.ping diff --git a/lib/gitlab_config.rb b/lib/gitlab_config.rb index 831f0e3..ebf72d6 100644 --- a/lib/gitlab_config.rb +++ b/lib/gitlab_config.rb @@ -54,26 +54,4 @@ class GitlabConfig def git_annex_enabled? @config['git_annex_enabled'] ||= false end - - # Build redis command to write update event in gitlab queue - def redis_command - if redis.empty? - # Default to old method of connecting to redis - # for users that haven't updated their configuration - %W(env -i redis-cli) - else - redis['database'] ||= 0 - redis['host'] ||= '127.0.0.1' - redis['port'] ||= '6379' - if redis.has_key?("socket") - %W(#{redis['bin']} -s #{redis['socket']} -n #{redis['database']}) - else - if redis.has_key?("pass") - %W(#{redis['bin']} -h #{redis['host']} -p #{redis['port']} -n #{redis['database']} -a #{redis['pass']}) - else - %W(#{redis['bin']} -h #{redis['host']} -p #{redis['port']} -n #{redis['database']}) - end - end - end - end end diff --git a/lib/gitlab_net.rb b/lib/gitlab_net.rb index 8b6d33b..8e1fe39 100644 --- a/lib/gitlab_net.rb +++ b/lib/gitlab_net.rb @@ -1,6 +1,7 @@ require 'net/http' require 'openssl' require 'json' +require 'redis' require_relative 'gitlab_config' require_relative 'gitlab_logger' @@ -63,6 +64,24 @@ class GitlabNet nil end + def redis_client + redis_config = config.redis + database = redis_config['database'] || 0 + params = { + host: redis_config['host'] || '127.0.0.1', + port: redis_config['port'] || 6379, + db: database + } + + if redis_config.has_key?("socket") + params = { path: redis_config['socket'], db: database } + elsif redis_config.has_key?("pass") + params[:password] = redis_config['pass'] + end + + Redis.new(params) + end + protected def config diff --git a/lib/gitlab_post_receive.rb b/lib/gitlab_post_receive.rb index 0fff479..8632432 100644 --- a/lib/gitlab_post_receive.rb +++ b/lib/gitlab_post_receive.rb @@ -2,6 +2,7 @@ require_relative 'gitlab_init' require_relative 'gitlab_net' require 'json' require 'base64' +require 'redis' require 'securerandom' class GitlabPostReceive @@ -74,11 +75,12 @@ class GitlabPostReceive queue = "#{config.redis_namespace}:queue:post_receive" msg = JSON.dump({ 'class' => 'PostReceive', 'args' => [@repo_path, @actor, changes], 'jid' => @jid }) - if system(*config.redis_command, 'rpush', queue, msg, - err: '/dev/null', out: '/dev/null') + + begin + GitlabNet.new.redis_client.rpush(queue, msg) return true - else - puts "GitLab: An unexpected error occurred (redis-cli returned #{$?.exitstatus})." + rescue => e + puts "GitLab: An unexpected error occurred in writing to Redis: #{e}" return false end end diff --git a/spec/gitlab_config_spec.rb b/spec/gitlab_config_spec.rb index 0ce641b..6c1af85 100644 --- a/spec/gitlab_config_spec.rb +++ b/spec/gitlab_config_spec.rb @@ -38,7 +38,7 @@ eos context 'remove trailing slashes' do before { config.send(:config)['gitlab_url'] = url + '//' } - + it { should eq(url) } end end @@ -48,40 +48,4 @@ eos it("returns false by default") { should eq(false) } end - - describe :redis_command do - subject { config.redis_command } - - context "with empty redis config" do - before do - config.stub(:redis) { {} } - end - - it { should be_an(Array) } - it { should include('redis-cli') } - end - - context "with host and port" do - before do - config.stub(:redis) { {'host' => 'localhost', 'port' => 1123, 'bin' => '/usr/bin/redis-cli'} } - end - - it { should be_an(Array) } - it { should include(config.redis['host']) } - it { should include(config.redis['bin']) } - it { should include(config.redis['port'].to_s) } - end - - context "with redis socket" do - let(:socket) { '/tmp/redis.socket' } - before do - config.stub(:redis) { {'bin' => '', 'socket' => socket } } - end - - it { should be_an(Array) } - it { should include(socket) } - it { should_not include('-p') } - it { should_not include('-h') } - end - end end diff --git a/spec/gitlab_net_spec.rb b/spec/gitlab_net_spec.rb index e4dee33..1312554 100644 --- a/spec/gitlab_net_spec.rb +++ b/spec/gitlab_net_spec.rb @@ -229,4 +229,44 @@ describe GitlabNet, vcr: true do store.should_receive(:add_path).with('test_path') end end + + describe '#redis_client' do + let(:config) { double('config') } + + context "with empty redis config" do + it 'returns default parameters' do + allow(gitlab_net).to receive(:config).and_return(config) + allow(config).to receive(:redis).and_return( {} ) + + expect_any_instance_of(Redis).to receive(:initialize).with({ host: '127.0.0.1', + port: 6379, + db: 0 }).and_call_original + gitlab_net.redis_client + end + end + + context "with host and port" do + it 'uses the specified host and port' do + allow(gitlab_net).to receive(:config).and_return(config) + allow(config).to receive(:redis).and_return( { 'host' => 'localhost', 'port' => 1123 } ) + + expect_any_instance_of(Redis).to receive(:initialize).with({ host: 'localhost', + port: 1123, + db: 0 }).and_call_original + gitlab_net.redis_client + end + end + + context "with redis socket" do + let(:socket) { '/tmp/redis.socket' } + + it 'uses the socket' do + allow(gitlab_net).to receive(:config).and_return(config) + allow(config).to receive(:redis).and_return( { 'socket' => socket }) + + expect_any_instance_of(Redis).to receive(:initialize).with({ path: socket, db: 0 }).and_call_original + gitlab_net.redis_client + end + end + end end diff --git a/spec/gitlab_post_receive_spec.rb b/spec/gitlab_post_receive_spec.rb index 9d7696e..0392bee 100644 --- a/spec/gitlab_post_receive_spec.rb +++ b/spec/gitlab_post_receive_spec.rb @@ -1,3 +1,4 @@ +# coding: utf-8 require 'spec_helper' require 'gitlab_post_receive' @@ -11,6 +12,7 @@ describe GitlabPostReceive do let(:repo_path) { File.join(repository_path, repo_name) + ".git" } let(:gitlab_post_receive) { GitlabPostReceive.new(repo_path, actor, wrongly_encoded_changes) } let(:message) { "test " * 10 + "message " * 10 } + let(:redis_client) { double('redis_client') } before do GitlabConfig.any_instance.stub(repos_path: repository_path) @@ -20,11 +22,11 @@ describe GitlabPostReceive do describe "#exec" do before do - GitlabConfig.any_instance.stub(redis_command: %w(env -i redis-cli)) - allow(gitlab_post_receive).to receive(:system).and_return(true) + allow_any_instance_of(GitlabNet).to receive(:redis_client).and_return(redis_client) end it "prints the broadcast message" do + expect(redis_client).to receive(:rpush) expect(gitlab_post_receive).to receive(:puts).ordered expect(gitlab_post_receive).to receive(:puts).with( "========================================================================" @@ -38,7 +40,7 @@ describe GitlabPostReceive do " message message message message message message message message" ).ordered - expect(gitlab_post_receive).to receive(:puts).ordered + expect(gitlab_post_receive).to receive(:puts).ordered expect(gitlab_post_receive).to receive(:puts).with( "========================================================================" ).ordered @@ -47,12 +49,9 @@ describe GitlabPostReceive do end it "pushes a Sidekiq job onto the queue" do - expect(gitlab_post_receive).to receive(:system).with( - *[ - *%w(env -i redis-cli rpush resque:gitlab:queue:post_receive), - %Q/{"class":"PostReceive","args":["#{repo_path}","#{actor}",#{base64_changes.inspect}],"jid":"#{gitlab_post_receive.jid}"}/, - { err: "/dev/null", out: "/dev/null" } - ] + expect(redis_client).to receive(:rpush).with( + 'resque:gitlab:queue:post_receive', + %Q/{"class":"PostReceive","args":["#{repo_path}","#{actor}",#{base64_changes.inspect}],"jid":"#{gitlab_post_receive.jid}"}/ ).and_return(true) gitlab_post_receive.exec @@ -61,7 +60,7 @@ describe GitlabPostReceive do context "when the redis command succeeds" do before do - allow(gitlab_post_receive).to receive(:system).and_return(true) + allow(redis_client).to receive(:rpush).and_return(true) end it "returns true" do @@ -72,8 +71,7 @@ describe GitlabPostReceive do context "when the redis command fails" do before do - allow(gitlab_post_receive).to receive(:system).and_return(false) - allow($?).to receive(:exitstatus).and_return(nil) + allow(redis_client).to receive(:rpush).and_raise('Fail') end it "returns false" do |