Skip to content
This repository was archived by the owner on Jul 19, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/cc/service/invocation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@
require 'cc/service/invocation/with_retries'
require 'cc/service/invocation/with_metrics'
require 'cc/service/invocation/with_error_handling'
require 'cc/service/invocation/with_return_values'

class CC::Service::Invocation
MIDDLEWARE = {
retries: WithRetries,
metrics: WithMetrics,
error_handling: WithErrorHandling,
return_values: WithReturnValues,
}

attr_reader :result
Expand Down
18 changes: 18 additions & 0 deletions lib/cc/service/invocation/with_return_values.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
class CC::Service::Invocation
class WithReturnValues
def initialize(invocation, message = nil)
@invocation = invocation
@message = message || "An internal error happened"
end

def call
result = @invocation.call
if result.nil?
{ ok: false, message: @message }
else
result
end
end
end
end

20 changes: 10 additions & 10 deletions lib/cc/services/github_pull_requests.rb
Original file line number Diff line number Diff line change
Expand Up @@ -85,19 +85,17 @@ def update_status_pending
end

def update_status(state, description)
if config.update_status
params = {
state: state,
description: description,
target_url: @payload["details_url"],
context: "codeclimate"
}
service_post(status_url, params.to_json)
end
params = {
state: state,
description: description,
target_url: @payload["details_url"],
context: "codeclimate"
}
service_post(status_url, params.to_json)
end

def add_comment
if config.add_comment && !comment_present?
if !comment_present?
body = {
body: COMMENT_BODY % @payload["compare_url"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was the add_comment check removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are actually relying on these checks to be here (and in #update_status) in our other PR 😄 #55

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay - we resolved our differences and are no longer relying on the config check that @jyurek removed here - so #55 should be compatible with this PR now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marshally Because the check actually happens here: https://github.com/codeclimate/codeclimate-services/blob/jy-do-not-allow-nil-branches/lib/cc/services/github_pull_requests.rb#L53 and this check only served to muddy the waters about what was happening.

}.to_json
Expand All @@ -106,6 +104,8 @@ def add_comment
doc = JSON.parse(response.body)
{ id: doc["id"] }
end
else
{ ok: false, message: "Comment already present" }
end
end

Expand Down
5 changes: 3 additions & 2 deletions test/github_pull_requests_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ def test_pull_request_status_error
commit_sha: "abc123",
state: "error",
})

end

def test_pull_request_status_test_success
Expand Down Expand Up @@ -115,11 +114,13 @@ def test_pull_request_comment_already_present

# With no POST expectation, test will fail if request is made.

receive_pull_request({ add_comment: true }, {
rsp = receive_pull_request({ add_comment: true, update_status: false }, {
github_slug: "pbrisbin/foo",
number: 1,
state: "success",
})
assert_not_nil rsp
assert_false rsp[:ok]
end

private
Expand Down
21 changes: 21 additions & 0 deletions test/invocation_return_values_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
require File.expand_path('../helper', __FILE__)

class InvocationReturnValuesTest < CC::Service::TestCase
def test_success_returns_upstream_result
handler = CC::Service::Invocation::WithReturnValues.new(
lambda { :return_value },
"error message"
)

assert_equal :return_value, handler.call
end

def test_empty_results_returns_hash
handler = CC::Service::Invocation::WithReturnValues.new(
lambda { nil },
"error message"
)

assert_equal( {ok: false, message: "error message"}, handler.call )
end
end
22 changes: 22 additions & 0 deletions test/invocation_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,28 @@ def test_success
assert_equal :some_result, result
end

def test_success_with_return_values
service = FakeService.new(:some_result)

result = CC::Service::Invocation.invoke(service) do |i|
i.with :return_values, "error"
end

assert_equal 1, service.receive_count
assert_equal :some_result, result
end

def test_failure_with_return_values
service = FakeService.new(nil)

result = CC::Service::Invocation.invoke(service) do |i|
i.with :return_values, "error"
end

assert_equal 1, service.receive_count
assert_equal( {ok: false, message: "error"}, result )
end

def test_retries
service = FakeService.new
service.fake_error = RuntimeError.new
Expand Down