-
Notifications
You must be signed in to change notification settings - Fork 9
bump version #77
bump version #77
Conversation
github_slug: "codeclimate/nillson", | ||
number: 33, | ||
commit_sha: "986ec903b8420f4e8c8d696d8950f7bd0667ff0c" | ||
# https://github.com/codeclimate/app/pull/1325 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use a test repo as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Made switch to a test repo.
LGTM |
# Usage: | ||
# | ||
# $ OAUTH_TOKEN="..." bundle exec ruby pull_request_test.rb | ||
# $ script SERVER REPO PR OAUTH_TOKEN SERVICE_TOKEN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right... is the script really invoked in this new manner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually are any changes to this script needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To unpack this a little bit:
The script should work as-is with either version of the services gem, so no change should be needed. The idea is it's here with example values that you modify locally to test whatever it is you need to test. If we checked in those modifications every time, the script would see unnecessary churn.
We did identify that the OAUTH_TOKEN
variable the script comment shows is a little mysterious, so I could see a change to further describe how to find a suitable value for that variable being valuable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, makes sense. I'll keep a comment about OAUTH_TOKEN
but revert the rest of script changes.
I think you can revert the script changes, but the version bump LGTM. |
…ary key to payload
ef5a549
to
8f105cd
Compare
@codeclimate/review Thanks for feedback. Ready for re-review. I reverted all changes to the script except two. The PR still:
Without
from module CC
class Service
class GitHubPullRequestsPresenter
include ActiveSupport::NumberHelper
def initialize(payload)
issue_comparison_counts = payload["issue_comparison_counts"]
if issue_comparison_counts
@fixed_count = issue_comparison_counts["fixed"]
@new_count = issue_comparison_counts["new"]
end
end and presenter gets created in def update_status_success
add_comment
update_status("success", presenter.success_message)
end
def update_status_failure
add_comment
update_status("failure", presenter.success_message)
end
def presenter
CC::Service::GitHubPullRequestsPresenter.new(@payload)
end |
LGTM |
@codeclimate/review This PR bumps the Code Climate Services version from 0.1.0 to 0.2.0, after a change to services and finalizer that lets cc-services now receive an optional
"messages"
key on@payload
- to provide clarity when analysis fails due to version incompatibility between snapshots. I have tested using the PR event trigger script incodeclimate-services
.The PR also makes minor updates to the pull request test, bringing over some clarifying instructions re OAUTH token from a same test in app.
If all looks well, I will bump the version and update
codeclimate/app
.One note:
Intent of change was to make error messages more explicit and remove details link when comparisons fail due to analysis version incompatibility between snapshots.
Default error message:

Analysis fails due to analysis version incompatibility error message:
The lengths of these messages were calculated with the above space in mind. However, when CodeClimate is the only service someone is using (might be rare), the set up for checks is different and less room is available for message, leading to it getting cut off: