Skip to content
This repository was archived by the owner on Jul 19, 2025. It is now read-only.

update failed pr message #76

Merged
merged 6 commits into from
Jul 2, 2015
Merged

update failed pr message #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
874028a
use message from payload in error status update
Jul 1, 2015
8b644ab
add message key to tests with error state
Jul 1, 2015
49dda72
set default error message as a constant. remove 'while' from old mess…
Jul 1, 2015
67271c5
use default error message when no message is provided. don't fetch me…
Jul 1, 2015
9ec9161
update tests
Jul 1, 2015
c239253
remove detail url from params if the status is error
Jul 1, 2015
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
12 changes: 9 additions & 3 deletions lib/cc/services/github_pull_requests.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,14 @@ class Config < CC::Service::Config
BASE_URL = "https://api.github.com"
BODY_REGEX = %r{<b>Code Climate</b> has <a href=".*">analyzed this pull request</a>}
COMMENT_BODY = '<img src="https://codeclimate.com/favicon.png" width="20" height="20" />&nbsp;<b>Code Climate</b> has <a href="%s">analyzed this pull request</a>.'

# Just make sure we can access GH using the configured token. Without
# additional information (github-slug, PR number, etc) we can't test much
# else.

MESSAGES = [
DEFAULT_ERROR = "Code Climate encountered an error attempting to analyze this pull request",
]

def receive_test
setup_http

Expand Down Expand Up @@ -87,8 +91,7 @@ def presenter
def update_status_error
update_status(
"error",
"Code Climate encountered an error while attempting to analyze this " +
"pull request."
@payload["message"] || DEFAULT_ERROR
)
end

Expand All @@ -104,6 +107,9 @@ def update_status(state, description)
target_url: @payload["details_url"],
context: "codeclimate"
}
if state == "error"
params.delete(:target_url)
Copy link
Member

Choose a reason for hiding this comment

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

At first glance, I don't love this. Why not just not include the target_url in the original payload. I don't think this class should be responsible for "overriding" the payload in this way. /c @gordondiggs @abaldwinhubter @pbrisbin

Copy link
Member

Choose a reason for hiding this comment

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

Typo. /cc @ABaldwinHunter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brynary apologies, just saw your note! I believe the thought was that removing the details_url would remove the details link from PRs that error out.

What's constructed in this update_status method doesn't match the original payload perfectly, and without adding in a conditional, it might set the target_url to nil when @payload["details_url"] gets left out.

That said, I think this is the place in finalizer where payload gets created and sent:

  module Finalizer
    class UpdatePullRequest
      def initialize(ref_point)
        @ref_point = ref_point
        @repo = ref_point.repo
        @pull_request = ref_point.pull_request
      end

      def update
        return unless updatable?

        emitter = EventEmitter.new
        emitter.emit(
          "pull_request",
          state: evaluation.state,
          issue_comparison_counts: {
            new: evaluation.new_issue_count,
            fixed: evaluation.fixed_issue_count,
          },
          details_url: evaluation.url,
          compare_url: evaluation.url,
          commit_sha: ref_point.commit_sha,
          github_slug: repo.github_slug,
          repo_id: repo.id.to_s,
          number: pull_request.number,
          message: evaluation.message,
        )
      end

We could perhaps make a change there.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the goal of removing the link in errored PRs makes sense (temporarily). I was only commenting on the implementation.

It seems like we could either change the payload that is sent to not include the URL (which would require a change elsewhere) or remove it here. What are your thoughts on how it should work?

—Bryan Helmkamp, Founder, Code Climate

bryan@brynary.com / 646-379-1810 / @brynary

Typed with thumbs.

On Mon, Jul 6, 2015 at 4:44 PM, Ashley Baldwin-Hunter
notifications@github.com wrote:

@@ -104,6 +107,9 @@ def update_status(state, description)
target_url: @payload["details_url"],
context: "codeclimate"
}

  •  if state == "error"

  •    params.delete(:target_url)
    @brynary apologies, just saw your note! I believe the thought was that removing the details_url would remove the details link from PRs that error out.
    What's constructed in this update_status method doesn't match the original payload perfectly, and without adding in a conditional, it might set the target_url to nil when @payload["details_url"] gets left out.
    That said, I think this is the place in finalizer where payload gets created and sent:
  module Finalizer
    class UpdatePullRequest
      def initialize(ref_point)
        @ref_point = ref_point
        @repo = ref_point.repo
        @pull_request = ref_point.pull_request
      end
      def update
        return unless updatable?
        emitter = EventEmitter.new
        emitter.emit(
          "pull_request",
          state: evaluation.state,
          issue_comparison_counts: {
            new: evaluation.new_issue_count,
            fixed: evaluation.fixed_issue_count,
          },
          details_url: evaluation.url,
          compare_url: evaluation.url,
          commit_sha: ref_point.commit_sha,
          github_slug: repo.github_slug,
          repo_id: repo.id.to_s,
          number: pull_request.number,
          message: evaluation.message,
        )
      end

We could perhaps make a change there.

Reply to this email directly or view it on GitHub:
https://github.com/codeclimate/codeclimate-services/pull/76/files#r33979971

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that change would be better in finalizer.

without adding in a conditional, it might set the target_url to nil when @payload["details_url"] gets left out.

I would test if this matters. If it does, I'd rather that conditional than the one you have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pbrisbin @brynary Hmm. It seems to me that right now, the codeclimate-services entity is not just getting a payload and relaying it, but making some additional calculations - e.g. formatting messages based on number of issues fixed, adding a context: 'codeclimate' to params passed to http request.

However, we recently updated finalizer to also do some analysis, including checking whether errored state is due to worker_version incompatibility and adding a unique message to payload, if that's the case.

On one hand, it seems nice for finalizer (or whatever other entity) to just be able to send a bunch of information to codeclimate-services github_PR and have cc-services know what to do with it.

On the other hand, it seems nice to have cc-services not know too much about how the stuff it's handling gets calculated - it just knows it needs to send message x to place y.

So, yeah. Agree. Ultimately I favor minimizing analysis in cc-services, which would mean not having it read state to decide whether to add details_url or not.

Copy link
Member

Choose a reason for hiding this comment

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

@ABaldwinHunter OK, can you please address this at your convenience?

end
@response = service_post(status_url, params.to_json)
end
end
Expand Down
18 changes: 17 additions & 1 deletion test/github_pull_requests_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,28 @@ def test_pull_request_status_success_generic
def test_pull_request_status_error
expect_status_update("pbrisbin/foo", "abc123", {
"state" => "error",
"description" => /encountered an error/,
"description" => CC::Service::GitHubPullRequests::DEFAULT_ERROR,
})

receive_pull_request({ update_status: true }, {
github_slug: "pbrisbin/foo",
commit_sha: "abc123",
state: "error",
message: nil,
})
end

def test_pull_request_status_error_message_provided
expect_status_update("pbrisbin/foo", "abc123", {
"state" => "error",
"description" => "descriptive message",
})

receive_pull_request({ update_status: true }, {
github_slug: "pbrisbin/foo",
commit_sha: "abc123",
state: "error",
message: "descriptive message",
})
end

Expand Down Expand Up @@ -112,6 +127,7 @@ def test_no_status_update_for_error_when_update_status_config_is_falsey
github_slug: "pbrisbin/foo",
commit_sha: "abc123",
state: "error",
message: nil,
})
end

Expand Down