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

Enable Webhooks for Pull Requests #746

Merged
merged 8 commits into from
Feb 29, 2016
Merged
Show file tree
Hide file tree
Changes from 2 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
12 changes: 6 additions & 6 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,18 @@ PATH
specs:
samson_pipelines (0.0.0)

PATH
remote: plugins/slack_webhooks
specs:
samson_slack_webhooks (0.0.0)
faraday

PATH
remote: plugins/slack
specs:
samson_slack (0.0.0)
slack-api (~> 1.1.0)

PATH
remote: plugins/slack_webhooks
specs:
samson_slack_webhooks (0.0.0)
faraday

PATH
remote: plugins/zendesk
specs:
Expand Down
22 changes: 17 additions & 5 deletions app/controllers/integrations/github_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ class Integrations::GithubController < Integrations::BaseController
cattr_accessor(:github_hook_secret) { ENV['GITHUB_HOOK_SECRET'] }

HMAC_DIGEST = OpenSSL::Digest.new('sha1')
WEBHOOK_HANDLERS = {
'push' => Changeset::CodePush,
'pull_request' => Changeset::PullRequest,
'issue_comment' => Changeset::IssueComment
}

protected

Expand All @@ -20,21 +25,28 @@ def valid_signature?
end

def valid_payload?
request.headers['X-Github-Event'] == 'push'
webhook_handler && webhook_handler.valid_webhook?(params)
end

def commit
params[:after]
webhook_event.sha
end

def branch
# Github returns full ref e.g. refs/heads/...
params[:ref][/refs\/heads\/(.+)/, 1]
webhook_event.branch
end

private

def service_type
'code'
webhook_event.service_type
end

def webhook_event
@webhook_event ||= webhook_handler.changeset_from_webhook(project, params)
end

def webhook_handler
WEBHOOK_HANDLERS[request.headers['X-Github-Event']]
end
end
9 changes: 9 additions & 0 deletions app/helpers/webhooks_helper.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
module WebhooksHelper
def webhook_sources(sources)
[
['Any CI', 'any_ci'],
['Any code push', 'any_code'],
['Any Pull Request', 'any_pull_request'],
['Any', 'any']
] + sources.map {|source| [ source.titleize, source ]}.to_a
end

def link_to_url(url)
link_to(url, url)
end
Expand Down
28 changes: 28 additions & 0 deletions app/models/changeset/code_push.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
class Changeset::CodePush
attr_reader :repo, :data

def initialize(repo, data)
@repo = repo
@data = data
end

def self.changeset_from_webhook(project, params = {})
new(project.github_repo, params)
end

def self.valid_webhook?(_)
true
end

def sha
data[:after]
end

def branch
data[:ref][/refs\/heads\/(.+)/, 1]
end

def service_type
'code' # Samson webhook category
end
end
36 changes: 36 additions & 0 deletions app/models/changeset/issue_comment.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
class Changeset::IssueComment
attr_reader :repo, :data

def initialize(repo, data)
@repo = repo
@body = data['body']
@data = data['issue']
end

def self.changeset_from_webhook(project, params = {})
new(project.github_repo, params)
end

def self.valid_webhook?(params)
comment = params[:comment] || {}
comment[:body] =~ Changeset::PullRequest::WEBHOOK_FILTER
end

def sha
pull_request.sha
end

def branch
pull_request.sha
Copy link
Contributor

Choose a reason for hiding this comment

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

use pull_request.branch ?

end

def service_type
'pull_request' # Samson webhook category
end

private

def pull_request
@pull_request ||= Changeset::PullRequest.find(repo, data['number'])
end
end
29 changes: 29 additions & 0 deletions app/models/changeset/pull_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ class Changeset::PullRequest
CODE_ONLY = "[A-Z][A-Z\\d]+-\\d+" # e.g., S4MS0N-123, SAM-456
PUNCT = "\\s|\\p{Punct}|~|="

WEBHOOK_FILTER = /(\[)\s*(samson)\s*(\])/i # [samson]
Copy link
Contributor

Choose a reason for hiding this comment

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

how about /\[\s*samson\s*\]/i ?


# Matches a markdown section heading named "Risks".
RISKS_SECTION = /^\s*#*\s*Risks?\s*#*\s*\n(?:\s*[-=]*\s*\n)?/i

Expand Down Expand Up @@ -31,6 +33,17 @@ def self.find(repo, number)
nil
end

def self.changeset_from_webhook(project, params = {})
find(project.github_repo, params[:number])
end

def self.valid_webhook?(params)
data = params[:pull_request] || {}
return false unless data[:state] == 'open'

data[:body] =~ WEBHOOK_FILTER || data.fetch(:base, {})[:ref] == 'production'
Copy link
Contributor

Choose a reason for hiding this comment

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

production PRs are always valid ?

Copy link
Contributor

Choose a reason for hiding this comment

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

the description said it would ignore production ... but this does the opposite ?

  • missing test ?
  • will cause problems when using other branches like staging to pick commits ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant in the description that it would ignore the "requirement" of having [samson] present. However, the more I think about this the more I'm thinking we should always require an explicit [samson] to be present. Thoughts @shajith?

Copy link

Choose a reason for hiding this comment

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

yeah i'm for requiring an explicit user action to kick this off

end

attr_reader :repo

def initialize(repo, data)
Expand All @@ -51,6 +64,18 @@ def reference
"##{number}"
end

def sha
@data.head.sha
end

def branch
@data.head.ref
end

def state
@data.state
end

def users
users = [@data.user, @data.merged_by]
users.compact.map {|user| Changeset::GithubUser.new(user) }.uniq
Expand All @@ -71,6 +96,10 @@ def jira_issues
@jira_issues ||= parse_jira_issues
end

def service_type
'pull_request' # Samson webhook category
end

private

def parse_risks(body)
Expand Down
3 changes: 1 addition & 2 deletions app/views/webhooks/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@
</div>

<div class="form-group">
<%= form.select :source, [['Any CI', 'any_ci'], ['Any code push', 'any_code'], ['Any', 'any']] +
@sources.map {|source| [ source.titleize, source ]}.to_a, {}, class: "form-control" %>
<%= form.select :source, webhook_sources(@sources), {}, class: "form-control" %>
</div>

<%= form.submit "Add webhook", class: "btn btn-primary" %>
Expand Down
62 changes: 46 additions & 16 deletions test/controllers/integrations/github_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@

before do
Deploy.delete_all

Integrations::GithubController.github_hook_secret = 'test'
end

project.webhooks.create!(stage: stages(:test_staging), branch: "dev", source: 'github')
does_not_deploy 'when the event is invalid' do
request.headers['X-Github-Event'] = 'event'
end

it 'does not deploy if signature is invalid' do
Expand All @@ -31,28 +32,57 @@
response.status.must_equal 200
end

it 'does not deploy if event is invalid' do
request.headers['X-Github-Event'] = 'event'

hmac = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), 'test', payload.to_param)
request.headers['X-Hub-Signature'] = "sha1=#{hmac}"
describe 'with a code push event' do
before do
request.headers['X-Github-Event'] = 'code_push'
project.webhooks.create!(stage: stages(:test_staging), branch: "dev", source: 'github')
end

post :create, payload.merge(token: project.token)
let(:user_name) { 'Github' }

project.deploys.must_equal []
response.status.must_equal 200
test_regular_commit "Github", no_mapping: { ref: 'refs/heads/foobar' }, failed: false do
request.headers['X-Github-Event'] = 'push'
hmac = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), 'test', payload.to_param)
request.headers['X-Hub-Signature'] = "sha1=#{hmac}"
end
end

describe 'with a valid signature' do
describe 'with a pull request event' do
before do
request.headers['X-Github-Event'] = 'pull_request'
project.webhooks.create!(stage: stages(:test_staging), branch: "dev", source: 'any_pull_request')
end

let(:user_name) { 'Github' }
let(:payload) do
{
ref: 'refs/heads/dev',
after: commit,
number: '42',
pull_request: {state: 'open', body: 'imafixwolves [samson]'}
}.with_indifferent_access
end
let(:api_response) do
stub({
user: stub(login: 'foo'),
merged_by: stub(login: 'bar'),
body: '',
head: stub(sha: commit, ref: 'refs/heads/dev')
})
end

before do
request.headers['X-Github-Event'] = 'push'
does_not_deploy 'with a non-open pull request state' do
payload.deep_merge!(pull_request: {state: 'closed'})
end

hmac = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), 'test', payload.to_param)
request.headers['X-Hub-Signature'] = "sha1=#{hmac}"
does_not_deploy 'without "[samson]" in the body' do
payload.deep_merge!(pull_request: {body: 'imafixwolves'})
end

test_regular_commit "Github", no_mapping: { ref: 'refs/heads/foobar' }, failed: false
# test_regular_commit "Github", no_mapping: { ref: 'refs/heads/foobar' }, failed: false do
Copy link
Contributor

Choose a reason for hiding this comment

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

delete or uncomment ?

# GITHUB.stubs(:pull_request).with('bar/foo', '42').returns(api_response)
# hmac = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), 'test', payload.to_param)
# request.headers['X-Hub-Signature'] = "sha1=#{hmac}"
# end
end
end
17 changes: 17 additions & 0 deletions test/models/changeset/pull_request_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
DataStruct = Struct.new(:user, :merged_by, :body, :title)
UserStruct = Struct.new(:login)

let(:project) { projects(:test) }
let(:data) { DataStruct.new(user, merged_by, body) }
let(:pr) { Changeset::PullRequest.new("xxx", data) }
let(:user) { UserStruct.new("foo") }
Expand All @@ -29,6 +30,22 @@
end
end

describe ".changeset_from_webhook" do
before do
Rails.cache.clear
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leftover from when the changeset_from_webhook was making a github api call. thanks.

end

it 'finds the pull request' do
GITHUB.stubs(:pull_request).with("bar/foo", 42).returns(data)
data.title = "Make it bigger!"

webhook_data = {number: 42, pull_request: {state: 'open'}}
pr = Changeset::PullRequest.changeset_from_webhook(project, webhook_data)

pr.title.must_equal "Make it bigger!"
end
end

describe "#users" do
it "returns the users associated with the pull request" do
pr.users.map(&:login).must_equal ["foo", "bar"]
Expand Down
15 changes: 15 additions & 0 deletions test/support/integration_controller_test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,19 @@ def test_regular_commit(user_name, options, &block)
end
end
end

def does_not_deploy(name, &block)
describe name do
before(&block) if block

it "does not deploy" do
hmac = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), 'test', payload.to_param)
request.headers['X-Hub-Signature'] = "sha1=#{hmac}"
post :create, payload.deep_merge(token: project.token)

project.deploys.must_equal []
response.status.must_equal 200
end
end
end
end