Merge branch 'ash.mckenzie/display-feedback-v2' into 'master'

Display helpful feedback when proxying an SSH git push to secondary request  (v2)

See merge request gitlab-org/gitlab-shell!246

9 files changed, 78 insertions, 88 deletions

diff --git a/lib/action/custom.rb b/lib/action/custom.rb
index 328a12f..a2f3d59 100644
--- a/lib/action/custom.rb
+++ b/lib/action/custom.rb
@@ -22,20 +22,15 @@ module Action
 
     def execute
       validate!
-      result = process_api_endpoints
-
-      if result && HTTP_SUCCESS_CODES.include?(result.code)
-        result
-      else
-        raise_unsuccessful!(result)
-      end
+      inform_client(info_message) if info_message
+      process_api_endpoints!
     end
 
     private
 
     attr_reader :gl_id, :payload
 
-    def process_api_endpoints
+    def process_api_endpoints!
       output = ''
       resp = nil
 
@@ -46,7 +41,14 @@ module Action
         json = { 'data' => data_with_gl_id, 'output' => output }
 
         resp = post(url, {}, headers: DEFAULT_HEADERS, options: { json: json })
-        return resp unless HTTP_SUCCESS_CODES.include?(resp.code)
+
+        # Net::HTTPSuccess is the parent of Net::HTTPOK, Net::HTTPCreated etc.
+        case resp
+        when Net::HTTPSuccess, Net::HTTPMultipleChoices
+          true
+        else
+          raise_unsuccessful!(resp)
+        end
 
         begin
           body = JSON.parse(resp.body)
@@ -54,8 +56,6 @@ module Action
           raise UnsuccessfulError, 'Response was not valid JSON'
         end
 
-        inform_client(body['message']) if body['message']
-
         print_flush(body['result'])
 
         # In the context of the git push sequence of events, it's necessary to read
@@ -78,6 +78,10 @@ module Action
       data['api_endpoints']
     end
 
+    def info_message
+      data['info_message']
+    end
+
     def config
       @config ||= GitlabConfig.new
     end
@@ -97,7 +101,11 @@ module Action
     end
 
     def inform_client(str)
-      $stderr.puts(str)
+      $stderr.puts(format_gitlab_output(str))
+    end
+
+    def format_gitlab_output(str)
+      str.split("\n").map { |line| "> GitLab: #{line}" }.join("\n")
     end
 
     def validate!
@@ -120,16 +128,17 @@ module Action
     end
 
     def raise_unsuccessful!(result)
-      message = begin
-        body = JSON.parse(result.body)
-        message = body['message']
-        message = Base64.decode64(body['result']) if !message && body['result'] && !body['result'].empty?
-        message ? message : NO_MESSAGE_TEXT
-      rescue JSON::ParserError
-        NO_MESSAGE_TEXT
-      end
+      message = "#{exception_message_for(result.body)} (#{result.code})"
+      raise UnsuccessfulError, format_gitlab_output(message)
+    end
+
+    def exception_message_for(body)
+      body = JSON.parse(body)
+      return body['message'] unless body['message'].to_s.empty?
 
-      raise UnsuccessfulError, "#{message} (#{result.code})"
+      body['result'].to_s.empty? ? NO_MESSAGE_TEXT : Base64.decode64(body['result'])
+    rescue JSON::ParserError
+      NO_MESSAGE_TEXT
     end
   end
 end
diff --git a/lib/gitlab_access_status.rb b/lib/gitlab_access_status.rb
index 8483863..dd6562e 100644
--- a/lib/gitlab_access_status.rb
+++ b/lib/gitlab_access_status.rb
@@ -1,8 +1,7 @@
 require 'json'
-require_relative 'http_codes'
 
 class GitAccessStatus
-  include HTTPCodes
+  HTTP_MULTIPLE_CHOICES = '300'.freeze
 
   attr_reader :message, :gl_repository, :gl_id, :gl_username, :gitaly, :git_protocol, :git_config_options, :payload
 
diff --git a/lib/gitlab_shell.rb b/lib/gitlab_shell.rb
index 79af861..57c70f5 100644
--- a/lib/gitlab_shell.rb
+++ b/lib/gitlab_shell.rb
@@ -95,8 +95,9 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
     $stderr.puts "GitLab: Invalid repository path"
     false
   rescue Action::Custom::BaseError => ex
-    $logger.warn('Custom action error', command: origin_cmd, user: log_username)
-    $stderr.puts "GitLab: #{ex.message}"
+    $logger.warn('Custom action error', exception: ex.class, message: ex.message,
+                                        command: origin_cmd, user: log_username)
+    $stderr.puts ex.message
     false
   end
 
diff --git a/lib/http_codes.rb b/lib/http_codes.rb
deleted file mode 100644
index 5f79095..0000000
--- a/lib/http_codes.rb
+++ /dev/null
@@ -1,8 +0,0 @@
-module HTTPCodes
-  HTTP_SUCCESS = '200'.freeze
-  HTTP_CREATED = '201'.freeze
-  HTTP_MULTIPLE_CHOICES = '300'.freeze
-  HTTP_UNAUTHORIZED = '401'.freeze
-  HTTP_NOT_FOUND = '404'.freeze
-  HTTP_SUCCESS_CODES = [HTTP_SUCCESS, HTTP_CREATED, HTTP_MULTIPLE_CHOICES].freeze
-end
diff --git a/lib/http_helper.rb b/lib/http_helper.rb
index ac18f49..b2a6211 100644
--- a/lib/http_helper.rb
+++ b/lib/http_helper.rb
@@ -1,10 +1,7 @@
-require_relative 'http_codes'
 require_relative 'httpunix'
 require_relative 'gitlab_logger'
 
 module HTTPHelper
-  include HTTPCodes
-
   READ_TIMEOUT = 300
   CONTENT_TYPE_JSON = 'application/json'.freeze
 
diff --git a/spec/action/custom_spec.rb b/spec/action/custom_spec.rb
index 78533c3..2ef7977 100644
--- a/spec/action/custom_spec.rb
+++ b/spec/action/custom_spec.rb
@@ -26,62 +26,57 @@ describe Action::Custom do
       end
 
       context 'that are valid' do
-        where(:primary_repo_data) do
-          [
-            [ 'http://localhost:3001/user1/repo1.git' ],
-            [{ 'http' => 'http://localhost:3001/user1/repo1.git' }],
-            [{ 'http' => 'http://localhost:3001/user1/repo1.git', 'ssh' => 'ssh://user@localhost:3002/user1/repo1.git' }]
-          ]
+        let(:payload) do
+          {
+            'action' => 'geo_proxy_to_primary',
+            'data' => {
+              'api_endpoints' => %w{/api/v4/fake/info_refs /api/v4/fake/push},
+              'primary_repo' => 'http://localhost:3001/user1/repo1.git'
+            }
+          }
         end
 
-        with_them do
-          let(:payload) do
-            {
-              'action' => 'geo_proxy_to_primary',
-              'data' => {
-                'api_endpoints' => %w{/api/v4/fake/info_refs /api/v4/fake/push},
-                'gl_username' => 'user1',
-                'primary_repo' => primary_repo_data
-              }
-            }
+        context 'and responds correctly' do
+          it 'prints a Base64 encoded result to $stdout' do
+            VCR.use_cassette("custom-action-ok") do
+              expect($stdout).to receive(:print).with('info_refs-result').ordered
+              expect($stdout).to receive(:print).with('push-result').ordered
+              subject.execute
+            end
           end
 
-          context 'and responds correctly' do
-            it 'prints a Base64 encoded result to $stdout' do
+          context 'with results printed to $stdout' do
+            before do
+              allow($stdout).to receive(:print).with('info_refs-result')
+              allow($stdout).to receive(:print).with('push-result')
+            end
+
+            it 'returns an instance of Net::HTTPCreated' do
               VCR.use_cassette("custom-action-ok") do
-                expect($stdout).to receive(:print).with('info_refs-result').ordered
-                expect($stdout).to receive(:print).with('push-result').ordered
-                subject.execute
+                expect(subject.execute).to be_instance_of(Net::HTTPCreated)
               end
             end
 
-            context 'with results printed to $stdout' do
+            context 'and with an information message provided' do
               before do
-                allow($stdout).to receive(:print).with('info_refs-result')
-                allow($stdout).to receive(:print).with('push-result')
+                payload['data']['info_message'] = 'Important message here.'
               end
 
-              it 'prints a message to $stderr' do
+              it 'prints said informational message to $stderr' do
                 VCR.use_cassette("custom-action-ok-with-message") do
-                  expect { subject.execute }.to output(/NOTE: Message here/).to_stderr
-                end
-              end
-
-              it 'returns an instance of Net::HTTPCreated' do
-                VCR.use_cassette("custom-action-ok") do
-                  expect(subject.execute ).to be_instance_of(Net::HTTPCreated)
+                  expect { subject.execute }.to output(/Important message here./).to_stderr
                 end
               end
             end
           end
+        end
 
-          context 'but responds incorrectly' do
-            it 'raises an UnsuccessfulError exception' do
-              VCR.use_cassette("custom-action-ok-not-json") do
-                expect {
-                  subject.execute
-                }.to raise_error(Action::Custom::UnsuccessfulError, 'Response was not valid JSON')
-              end
+        context 'but responds incorrectly' do
+          it 'raises an UnsuccessfulError exception' do
+            VCR.use_cassette("custom-action-ok-not-json") do
+              expect do
+                subject.execute
+              end.to raise_error(Action::Custom::UnsuccessfulError, 'Response was not valid JSON')
             end
           end
         end
@@ -93,7 +88,6 @@ describe Action::Custom do
             {
               'action' => 'geo_proxy_to_primary',
               'data' => {
-                'gl_username' => 'user1',
                 'primary_repo' => 'http://localhost:3001/user1/repo1.git'
               }
             }
@@ -110,7 +104,6 @@ describe Action::Custom do
               'action' => 'geo_proxy_to_primary',
               'data' => {
                 'api_endpoints' => [],
-                'gl_username' => 'user1',
                 'primary_repo' => 'http://localhost:3001/user1/repo1.git'
               }
             }
@@ -135,7 +128,6 @@ describe Action::Custom do
               'action' => 'geo_proxy_to_primary',
               'data' => {
                 'api_endpoints' => %w{/api/v4/fake/info_refs_bad /api/v4/fake/push_bad},
-                'gl_username' => 'user1',
                 'primary_repo' => 'http://localhost:3001/user1/repo1.git'
               }
             }
@@ -144,9 +136,9 @@ describe Action::Custom do
           context 'and response is JSON' do
             it 'raises an UnsuccessfulError exception' do
               VCR.use_cassette("custom-action-not-ok-json") do
-                expect {
+                expect do
                   subject.execute
-                }.to raise_error(Action::Custom::UnsuccessfulError, 'You cannot perform write operations on a read-only instance (403)')
+                end.to raise_error(Action::Custom::UnsuccessfulError, '> GitLab: You cannot perform write operations on a read-only instance (403)')
               end
             end
           end
@@ -154,9 +146,9 @@ describe Action::Custom do
           context 'and response is not JSON' do
             it 'raises an UnsuccessfulError exception' do
               VCR.use_cassette("custom-action-not-ok-not-json") do
-                expect {
+                expect do
                   subject.execute
-                }.to raise_error(Action::Custom::UnsuccessfulError, 'No message (403)')
+                end.to raise_error(Action::Custom::UnsuccessfulError, '> GitLab: No message (403)')
               end
             end
           end
diff --git a/spec/gitlab_access_spec.rb b/spec/gitlab_access_spec.rb
index c9922dd..92268e2 100644
--- a/spec/gitlab_access_spec.rb
+++ b/spec/gitlab_access_spec.rb
@@ -8,7 +8,7 @@ describe GitlabAccess do
   let(:api) do
     double(GitlabNet).tap do |api|
       allow(api).to receive(:check_access).and_return(GitAccessStatus.new(true,
-                                                HTTPCodes::HTTP_SUCCESS,
+                                                '200',
                                                  'ok',
                                                  gl_repository: 'project-1',
                                                  gl_id: 'user-123',
@@ -46,7 +46,7 @@ describe GitlabAccess do
       before do
         allow(api).to receive(:check_access).and_return(GitAccessStatus.new(
                   false,
-                  HTTPCodes::HTTP_UNAUTHORIZED,
+                  '401',
                   'denied',
                   gl_repository: nil,
                   gl_id: nil,
diff --git a/spec/gitlab_shell_spec.rb b/spec/gitlab_shell_spec.rb
index 77fb6cd..eef572e 100644
--- a/spec/gitlab_shell_spec.rb
+++ b/spec/gitlab_shell_spec.rb
@@ -25,7 +25,7 @@ describe GitlabShell do
   let(:gitaly_check_access) do
     GitAccessStatus.new(
       true,
-      HTTPCodes::HTTP_SUCCESS,
+      '200',
       'ok',
       gl_repository: gl_repository,
       gl_id: gl_id,
@@ -41,7 +41,7 @@ describe GitlabShell do
       allow(api).to receive(:discover).and_return({ 'name' => 'John Doe', 'username' => 'testuser' })
       allow(api).to receive(:check_access).and_return(GitAccessStatus.new(
                 true,
-                HTTPCodes::HTTP_SUCCESS,
+                '200',
                 'ok',
                 gl_repository: gl_repository,
                 gl_id: gl_id,
@@ -262,7 +262,7 @@ describe GitlabShell do
         let(:custom_action_gitlab_access_status) do
           GitAccessStatus.new(
             true,
-            HTTPCodes::HTTP_MULTIPLE_CHOICES,
+            '300',
             'Multiple Choices',
             payload: fake_payload
           )
diff --git a/spec/vcr_cassettes/custom-action-ok-with-message.yml b/spec/vcr_cassettes/custom-action-ok-with-message.yml
index c2dbd58..9d44a37 100644
--- a/spec/vcr_cassettes/custom-action-ok-with-message.yml
+++ b/spec/vcr_cassettes/custom-action-ok-with-message.yml
@@ -46,7 +46,7 @@ http_interactions:
       - '1.436040'
     body:
       encoding: UTF-8
-      string: '{"result":"aW5mb19yZWZzLXJlc3VsdA==\n", "message":"NOTE: Message here"}'
+      string: '{"result":"aW5mb19yZWZzLXJlc3VsdA==\n"}'
     http_version:
   recorded_at: Fri, 20 Jul 2018 06:18:58 GMT
 - request: