update failed pr message

[ABaldwinHunter]

@codeclimate/review WIP. I wanted to create a more customized message for PR's that error due to incompatible worker versions- particularly engine related.

So, my solution was:

1. add a key to payload that specifies if the worker version was changed (also updated in finalizer)
   PR: https://github.com/codeclimate/finalizer/pull/49
2. create two constants for error messages: one default, one specific to worker version issue.

EDIT: Feedback welcome. It's my first time working with codeclimate-services. If my approach seems right, I'll add tests.

[ABaldwinHunter]

@codeclimate/review I thought this PR was ready re-review - but now am not sure why Travis Build is failing (exploring). Tests pass locally when I run bundle exec rake.

It updates pull_requests to expect a payload with a [:message] key that is only used if the state is an error.

Goal is to provide clearer feedback to users when PR fails due to incompatible worker versions.

First draft message in that case: "Whoops. Code Climate cannot analyze a pull request against its base when only one is configured to use engines. We apologize for the inconvenience."

See related changes in finalizer: https://github.com/codeclimate/finalizer/pull/50

[pbrisbin]

What do you think about @payload["message"] || DEFAULT_ERROR_MESSAGE?

I think generally, all the update_status_x methods should use @payload["message"] || SOME_DEFAULT and let the callers be in charge of it (not the presenter here), but that's a bigger change and we can punt on it for now.

[ABaldwinHunter]

@pbrisbin okay that's fine by me. I see some reasons supporting both places as the site of setting a message_default - maybe it makes sense to have messages get set in one place, so finalizer should worry about the messages and services can just eat them. But on the other hand, I think having a default message on services side is fine.

Generally, agree that it seems a little confusing that the presenter is used for some messages and not others (and the method is named success_message even when the status is failure :p ). But punting sounds ideal to me. :) Keep changes separate.

[pbrisbin]

Agreed. cc-services should not be in the business of crafting messages based on data, etc. But having a sane, generic default seems totally reasonable. And is important for something like this which is a shared, open source gem that has to think about backwards compatibility.

[ABaldwinHunter]

Mmmm makes sense.

Imagine we'll probably also want to avoid fetching the message then.

[pbrisbin]

Yup, my example snippet should work and make the new #message method unused (or keep it but use Hash#[] instead).

[ABaldwinHunter]

@codeclimate/review I added the changes discussed above, including adding a DEFAULT_ERROR message constant.

New change that might need improvement: my naive attempt to remove the Details link was to remove the details url (target_url) from getting passed as params when the status was "error". Wasn't sure if that was the correct approach. Was also wondering best way to go about testing this sort of change.

[gdiggs]

👍 LGTM

[brynary]

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

[brynary]

Typo. /cc @ABaldwinHunter

[ABaldwinHunter]

@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.

[brynary]

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

[pbrisbin]

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.

[ABaldwinHunter]

@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.

[brynary]

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