summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorNick Thomas <nick@gitlab.com>2018-09-25 14:26:35 +0000
committerNick Thomas <nick@gitlab.com>2018-09-25 14:26:35 +0000
commit1cc2993f357c4467e4d45c54c01d2307103efb3e (patch)
tree990d233c522bfbcba0a1000ce6d31e4b078fd8aa
parentdd54651b987c473fc42ae6c81daf0a47a92f9dca (diff)
parent5f12a6a41a46800841ba02e72a6a4eb6d5e129ae (diff)
downloadgitlab-shell-1cc2993f357c4467e4d45c54c01d2307103efb3e.tar.gz
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
-rw-r--r--lib/action/custom.rb51
-rw-r--r--lib/gitlab_access_status.rb3
-rw-r--r--lib/gitlab_shell.rb5
-rw-r--r--lib/http_codes.rb8
-rw-r--r--lib/http_helper.rb3
-rw-r--r--spec/action/custom_spec.rb84
-rw-r--r--spec/gitlab_access_spec.rb4
-rw-r--r--spec/gitlab_shell_spec.rb6
-rw-r--r--spec/vcr_cassettes/custom-action-ok-with-message.yml2
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: