Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GitHub pull request review #702

Merged
merged 22 commits into from Jan 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
2ac42ef
Working on github review integration;
Antondomashnev Dec 27, 2016
38465ee
Playing around with github_review integration;
Antondomashnev Jan 2, 2017
528494b
Working on Octokit PullRequestReviews extension;
Antondomashnev Jan 8, 2017
c3b362d
Create basic methods for PullRequestReivews for Octokit;
Antondomashnev Jan 8, 2017
a3c089d
Fixes in sytax and logic in github & github_review classes;
Antondomashnev Jan 8, 2017
3cf1015
Working on github review transitions resolver;
Antondomashnev Jan 9, 2017
9c3165a
Add tests for review method for github request_source;
Antondomashnev Jan 11, 2017
fbb4027
Add tests for review method for github request_source;
Antondomashnev Jan 11, 2017
cb6e0c4
Add tests for github_review_resolver;
Antondomashnev Jan 11, 2017
f427bee
Update review resolver with new logic;
Antondomashnev Jan 19, 2017
8f6daa9
Do not submit pull request review with the same body;
Antondomashnev Jan 19, 2017
8ec776a
Cleanup GitHubReview;
Antondomashnev Jan 20, 2017
6e612c6
Create ReviewResolver tests;
Antondomashnev Jan 20, 2017
b0149e0
Working on review spec;
Antondomashnev Jan 20, 2017
898e6a9
Fix GitHubReview specs;
Antondomashnev Jan 21, 2017
b5bd8f6
Finalize spec for github review;
Antondomashnev Jan 22, 2017
b07ef94
Small cleanup in review object;
Antondomashnev Jan 22, 2017
d4a20ab
Fix testd and offenses;
Antondomashnev Jan 22, 2017
5730d68
PR feedback; Test fixes;
Antondomashnev Jan 22, 2017
c60b63f
Do not show pull request review summary message;
Antondomashnev Jan 24, 2017
68474f0
Update changelog and documentation;
Antondomashnev Jan 24, 2017
4500e98
Downgrade rubocop to 0.46;
Antondomashnev Jan 25, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
@@ -1,6 +1,6 @@
## master

* Add your own contribution below
* PR Review in Beta. Provides access to creating a GitHub Review instead of a typical GitHub comment - antondomashnev

## 4.0.5

Expand Down
2 changes: 1 addition & 1 deletion danger.gemspec
Expand Up @@ -39,7 +39,7 @@ Gem::Specification.new do |spec|
spec.add_development_dependency "pry", "~> 0.10"
spec.add_development_dependency "pry-byebug"

spec.add_development_dependency "rubocop", "~> 0.44"
spec.add_development_dependency "rubocop", "~> 0.46.0"
spec.add_development_dependency "yard", "~> 0.8"

spec.add_development_dependency "listen", "3.0.7"
Expand Down
2 changes: 1 addition & 1 deletion lib/danger/ci_source/bitrise.rb
@@ -1,5 +1,5 @@
# http://devcenter.bitrise.io/docs/available-environment-variables
require "danger/request_sources/github"
require "danger/request_sources/github/github"
require "danger/request_sources/gitlab"

module Danger
Expand Down
2 changes: 1 addition & 1 deletion lib/danger/ci_source/buildkite.rb
@@ -1,6 +1,6 @@
# https://buildkite.com/docs/agent/osx
# https://buildkite.com/docs/guides/environment-variables
require "danger/request_sources/github"
require "danger/request_sources/github/github"
require "danger/request_sources/gitlab"

module Danger
Expand Down
2 changes: 1 addition & 1 deletion lib/danger/ci_source/circle.rb
@@ -1,7 +1,7 @@
# https://circleci.com/docs/environment-variables
require "uri"
require "danger/ci_source/circle_api"
require "danger/request_sources/github"
require "danger/request_sources/github/github"

module Danger
# ### CI Setup
Expand Down
2 changes: 1 addition & 1 deletion lib/danger/ci_source/drone.rb
@@ -1,5 +1,5 @@
# http://readme.drone.io/usage/variables/
require "danger/request_sources/github"
require "danger/request_sources/github/github"
require "danger/request_sources/gitlab"

module Danger
Expand Down
2 changes: 1 addition & 1 deletion lib/danger/ci_source/jenkins.rb
@@ -1,6 +1,6 @@
# https://wiki.jenkins-ci.org/display/JENKINS/Building+a+software+project#Buildingasoftwareproject-JenkinsSetEnvironmentVariables
# https://wiki.jenkins-ci.org/display/JENKINS/GitHub+pull+request+builder+plugin
require "danger/request_sources/github"
require "danger/request_sources/github/github"
require "danger/request_sources/gitlab"
require "danger/request_sources/bitbucket_server"
require "danger/request_sources/bitbucket_cloud"
Expand Down
2 changes: 1 addition & 1 deletion lib/danger/ci_source/local_git_repo.rb
Expand Up @@ -3,7 +3,7 @@
require "git"
require "uri"

require "danger/request_sources/github"
require "danger/request_sources/github/github"

require "danger/ci_source/support/find_repo_info_from_url"
require "danger/ci_source/support/find_repo_info_from_logs"
Expand Down
2 changes: 1 addition & 1 deletion lib/danger/ci_source/semaphore.rb
@@ -1,5 +1,5 @@
# https://semaphoreci.com/docs/available-environment-variables.html
require "danger/request_sources/github"
require "danger/request_sources/github/github"

module Danger
# ### CI Setup
Expand Down
2 changes: 1 addition & 1 deletion lib/danger/ci_source/surf.rb
@@ -1,5 +1,5 @@
# http://github.com/surf-build/surf
require "danger/request_sources/github"
require "danger/request_sources/github/github"

module Danger
# ### CI Setup
Expand Down
2 changes: 1 addition & 1 deletion lib/danger/ci_source/teamcity.rb
@@ -1,5 +1,5 @@
# https://www.jetbrains.com/teamcity/
require "danger/request_sources/github"
require "danger/request_sources/github/github"
require "danger/request_sources/gitlab"

module Danger
Expand Down
2 changes: 1 addition & 1 deletion lib/danger/ci_source/travis.rb
@@ -1,6 +1,6 @@
# http://docs.travis-ci.com/user/osx-ci-environment/
# http://docs.travis-ci.com/user/environment-variables/
require "danger/request_sources/github"
require "danger/request_sources/github/github"

module Danger
# ### CI Setup
Expand Down
2 changes: 1 addition & 1 deletion lib/danger/ci_source/xcode_server.rb
@@ -1,6 +1,6 @@
# Following the advice from @czechboy0 https://github.com/danger/danger/issues/171
# https://github.com/czechboy0/Buildasaur
require "danger/request_sources/github"
require "danger/request_sources/github/github"

module Danger
# ### CI Setup
Expand Down
21 changes: 21 additions & 0 deletions lib/danger/danger_core/plugins/dangerfile_github_plugin.rb
Expand Up @@ -92,6 +92,27 @@ def self.instance_name
"github"
end

# @!group PR Review
#
Copy link
Member

Choose a reason for hiding this comment

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

This will need considerable documentation before we ship ( as this is what an end user would see on the website ) - I'm happy to do this once you're ready though 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, do you think we should provide an example here or what are your thoughts? 😄

Copy link
Member

Choose a reason for hiding this comment

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

Sure thing, something like this

# @!group PR Review
#
# In Beta. Provides access to creating a GitHub Review instead of a typical GitHub comment.
#
# To use you announce the start of your review, and the end via the `start` and `submit` functions,
# for example:
#
# ```
#   github.review.start
#   github.review.fail("Please add a CHANGELOG entry") if has_no_changelog
#   github.review.warn("Highway to the Danger Zone") if pr_includes_word_danger
#   github.review.message("You might want to read #{url}") if may_require_docs
#   github.review.submit
# ```
#
# @return [GitHubReview]

Copy link
Member

Choose a reason for hiding this comment

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

People don't see the inline documentation for the review object, so it needs to be attached to this DSL attribute to end up on the reference - http://danger.systems/reference.html

Copy link
Member Author

Choose a reason for hiding this comment

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

@orta looks cool 👍
The small feedback:

  1. @return [GitHubReview] -> @return [GitHubSource::Review] or we can just leave Review.
  2. Should we mention github.review.markdown(...) ?

Copy link
Member

Choose a reason for hiding this comment

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

@return [GitHubReview] -> @return [GitHubSource::Review] or we can just leave Review.

IMO @return Review is fine, maybe even @return ReviewDSL as these docs are for end-users who don't care about our internal semantics

Should we mention github.review.markdown(...) ?

Yep :D - please make it a real-ish example like the ones above

# In Beta. Provides access to creating a GitHub Review instead of a typical GitHub comment.
#
# To use you announce the start of your review, and the end via the `start` and `submit` functions,
# for example:
#
# ```
# github.review.start
# github.review.fail("Please add a CHANGELOG entry") if has_no_changelog
# github.review.warn("Highway to the Danger Zone") if pr_includes_word_danger
# github.review.message("You might want to read #{url}") if may_require_docs
# github.review.markdown("Please update your changelog entry according an #{example}") if changelog_format_not_valid
# github.review.submit
# ```
#
# @return [ReviewDSL]
def review
@github.review
end

# @!group PR Metadata
# The title of the Pull Request.
# @return [String]
Expand Down
Expand Up @@ -2,7 +2,8 @@
require "octokit"
require "danger/helpers/comments_helper"
require "danger/helpers/comment"

require "danger/request_sources/github/github_review"
require "danger/request_sources/github/octokit_pr_review.rb"
require "danger/request_sources/support/get_ignored_violation"

module Danger
Expand Down Expand Up @@ -63,6 +64,16 @@ def pr_diff
@pr_diff ||= client.pull_request(ci_source.repo_slug, ci_source.pull_request_id, accept: "application/vnd.github.v3.diff")
end

def review
return @review unless @review.nil?
@review = client.pull_request_reviews(ci_source.repo_slug, ci_source.pull_request_id)
.map { |review_json| Danger::RequestSources::GitHubSource::Review.new(client, ci_source, review_json) }
.select(&:generated_by_danger?)
.last
@review ||= Danger::RequestSources::GitHubSource::Review.new(client, ci_source)
@review
end

def setup_danger_branches
# we can use a github specific feature here:
base_commit = self.pr_json["base"]["sha"]
Expand Down
126 changes: 126 additions & 0 deletions lib/danger/request_sources/github/github_review.rb
@@ -0,0 +1,126 @@
# coding: utf-8
require "octokit"
require "danger/ci_source/ci_source"
require "danger/request_sources/github/octokit_pr_review"
require "danger/request_sources/github/github_review_resolver"
require "danger/danger_core/messages/violation"
require "danger/danger_core/messages/markdown"
require "danger/helpers/comments_helper"
require "danger/helpers/comment"

module Danger
module RequestSources
module GitHubSource
class Review
include Danger::Helpers::CommentsHelper

# @see https://developer.github.com/v3/pulls/reviews/ for all possible events
EVENT_APPROVE = "APPROVE".freeze
EVENT_REQUEST_CHANGES = "REQUEST_CHANGES".freeze
EVENT_COMMENT = "COMMENT".freeze

# Current review status, if the review has not been submitted yet -> STATUS_PENDING
STATUS_APPROVED = "APPROVED".freeze
STATUS_REQUESTED_CHANGES = "CHANGES_REQUESTED".freeze
STATUS_COMMENTED = "COMMENTED".freeze
STATUS_PENDING = "PENDING".freeze

attr_reader :id, :body, :status, :review_json

def initialize(client, ci_source, review_json = nil)
@ci_source = ci_source
@client = client
@review_json = review_json
end

def id
return nil unless self.review_json
self.review_json["id"]
end

def body
return "" unless self.review_json
self.review_json["body"]
end

def status
return STATUS_PENDING if self.review_json.nil?
return self.review_json["state"]
end

# Starts the new review process
def start
@warnings = []
@errors = []
@messages = []
@markdowns = []
end

# Submits the prepared review
def submit
general_violations = generate_general_violations
submission_body = generate_body

# If the review resolver says that there is nothing to submit we skip submission
return unless ReviewResolver.should_submit?(self, submission_body)

@review_json = @client.create_pull_request_review(@ci_source.repo_slug, @ci_source.pull_request_id, generate_event(general_violations), submission_body)
end

def generated_by_danger?(danger_id = "danger")
self.review_json["body"].include?("generated_by_#{danger_id}")
end

def message(message, sticky = true, file = nil, line = nil)
@messages << Violation.new(message, sticky, file, line)
end

def warn(message, sticky = true, file = nil, line = nil)
@warnings << Violation.new(message, sticky, file, line)
end

def fail(message, sticky = true, file = nil, line = nil)
@errors << Violation.new(message, sticky, file, line)
end

def markdown(message, file = nil, line = nil)
@markdowns << Markdown.new(message, file, line)
end

private

# The only reason to request changes for the PR is to have errors from Danger
# otherwise let's just notify user and we're done
def generate_event(violations)
violations[:errors].empty? ? EVENT_APPROVE : EVENT_REQUEST_CHANGES
end

def generate_body(danger_id: "danger")
previous_violations = parse_comment(body)
general_violations = generate_general_violations
new_body = generate_comment(warnings: general_violations[:warnings],
errors: general_violations[:errors],
messages: general_violations[:messages],
markdowns: general_violations[:markdowns],
previous_violations: previous_violations,
danger_id: danger_id,
template: "github")
return new_body
end

def generate_general_violations
general_warnings = @warnings.reject(&:inline?)
general_errors = @errors.reject(&:inline?)
general_messages = @messages.reject(&:inline?)
general_markdowns = @markdowns.reject(&:inline?)
{
warnings: general_warnings,
markdowns: general_markdowns,
errors: general_errors,
messages: general_messages
}
end
end
end
end
end
18 changes: 18 additions & 0 deletions lib/danger/request_sources/github/github_review_resolver.rb
@@ -0,0 +1,18 @@
# coding: utf-8
require "danger/request_sources/github/github_review"

module Danger
module RequestSources
module GitHubSource
class ReviewResolver
def self.should_submit?(review, body)
return !same_body?(body, review.body)
end

def self.same_body?(body1, body2)
return !body1.nil? && !body2.nil? && body1 == body2
end
end
end
end
end
83 changes: 83 additions & 0 deletions lib/danger/request_sources/github/octokit_pr_review.rb
@@ -0,0 +1,83 @@
# coding: utf-8
require "octokit"

module Octokit
class Client
# The Pull Request Review API is currently available for developers to preview.
# During the preview period, the API may change without advance notice.
# please see the blog post for full details.
# To access the API during the preview period, you must provide
# a custom media type in the Accept header:
CUSTOM_ACCEPT_HEADER = "application/vnd.github.black-cat-preview+json".freeze

# Approve pull request event
PULL_REQUEST_REVIEW_EVENT_APPROVE = "APPROVE".freeze

# Request changes on the pull request event
PULL_REQUEST_REVIEW_EVENT_REQUEST_CHANGES = "REQUEST_CHANGES".freeze

# Left a gemeneral comment on the pull request event
PULL_REQUEST_REVIEW_EVENT_COMMENT = "COMMENT".freeze

# List pull request reviews for a pull request
#
# @see https://developer.github.com/v3/pulls/reviews/#list-reviews-on-a-pull-request
# @param repo [Integer, String, Hash, Repository] A GitHub repository
# @param pull_request_number [Integer] Number of the pull request to fetch reviews for
# @return [Array<Sawyer::Resource>] Array of reviews
# @example
# Octokit.pull_request_reviews('rails/rails', :state => 'closed')
def pull_request_reviews(repo, pull_request_number, options = {})
accept = {
accept: CUSTOM_ACCEPT_HEADER
}
paginate "#{Repository.path repo}/pulls/#{pull_request_number}/reviews", options.merge(accept)
end

# Create a pull request review
#
# @see https://developer.github.com/v3/pulls/reviews/#create-a-pull-request-review
# @param repo [Integer, String, Hash, Repository] A GitHub repository
# @param pull_request_number [Integer] Number of the pull request to create review for
# @param event [String] The review action (event) to perform. Can be one of
# PULL_REQUEST_REVIEW_EVENT_APPROVE
# PULL_REQUEST_REVIEW_EVENT_REQUEST_CHANGES
# PULL_REQUEST_REVIEW_EVENT_COMMENT
# @param body [String] The body for the pull request review (optional). Supports GFM.
# @return [Sawyer::Resource] The newly created pull request
# @example
# @client.create_pull_request_review("octokit/octokit.rb", "APPROVE", "Thanks for your contribution")
def create_pull_request_review(repo, pull_request_number, event, body = nil, options = {})
review = {
event: event,
accept: CUSTOM_ACCEPT_HEADER
}
review[:body] = body unless body.nil?
post "#{Repository.path repo}/pulls/#{pull_request_number}/reviews", options.merge(review)
end

# Submit a pull request review
#
# @see https://developer.github.com/v3/pulls/reviews/#create-a-pull-request-review
# @param repo [Integer, String, Hash, Repository] A GitHub repository
# @param pull_request_number [Integer] Number of the pull request to create review for
# @param review_id [Integer] ID of the pull request review to submit
# @param event [String] The review action (event) to perform. Can be one of
# PULL_REQUEST_REVIEW_EVENT_APPROVE
# PULL_REQUEST_REVIEW_EVENT_REQUEST_CHANGES
# PULL_REQUEST_REVIEW_EVENT_COMMENT
# @param body [String] The body for the pull request review (optional). Supports GFM.
# @return [Sawyer::Resource] The newly created pull request
# @example
# @client.submit_pull_request_review("octokit/octokit.rb", "REQUEST_CHANGES",
# "Nice changes, but please make couple of imrovements")
def submit_pull_request_review(repo, pull_request_number, review_id, event, body = nil, options = {})
review = {
event: event,
accept: CUSTOM_ACCEPT_HEADER
}
review[:body] = body unless body.nil?
post "#{Repository.path repo}/pulls/#{pull_request_number}/reviews/#{review_id}/events", options.merge(review)
end
end
end
Copy link
Member

Choose a reason for hiding this comment

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

nice one