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

Enable Webhooks for Pull Requests #746

merged 8 commits into from
Feb 29, 2016

Conversation

mwerner
Copy link
Contributor

@mwerner mwerner commented Feb 23, 2016

This PR will add functionality to the github webhook. It expands the types of webhook types Samson will respond to beyond Code Push. It uses the ChangeSet namespace to establish a common protocol to be a webhook handler. The GithubController then delegates the method calls to the webhook handler, and uses the returned info to decide how to respond.

  1. Adding a "Any Pull Request" webhook will start watching webhooks for pull requests, and the issue comments present on those pull requests.
  2. I have hardcoded the requirement of [samson] to be present in the body of either the Pull Request or Issue Comment.
  3. If the base for the pull request is production Samson will ignore the [samson] requirement.

@zendesk/samson

@mwerner mwerner mentioned this pull request Feb 23, 2016
@grosser
Copy link
Contributor

grosser commented Feb 23, 2016

can you elaborate on what this will be used for / what this enabled or fixes ?

@mwerner
Copy link
Contributor Author

mwerner commented Feb 23, 2016

The intent is to be able to build and upload assets on Pull requests that are deemed by those involved as something worth testing. Currently they need to copy their sha to Samson. I am aiming to expedite that process by watching for an explicit [samson] reference.

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

@grosser
Copy link
Contributor

grosser commented Feb 25, 2016

so when I push multiple times, this will run multiple times ?

sounds like a useful feature ...

@mwerner
Copy link
Contributor Author

mwerner commented Feb 25, 2016

This is separate from the code push webhook.

In a hypothetical example:
You can open a PR with [samson] in the body of the PR description and have your webhook run. Push a few commits and discuss various parts of the code without triggering any webhooks. Then write an issue comment with [samson] in it and fire your webhook with the latest sha on the PR.

@mwerner
Copy link
Contributor Author

mwerner commented Feb 26, 2016

I noticed some behavior that was slightly different than I expected, but I think I'm in favor of. If you put [samson] in your PR description, every subsequent code push will cause a pull request sync webhook which will pass the [samson] in the PR body test, causing a deploy.

If you only include [samson] in a comment, it will trigger a webhook once for the latest sha on that pull request, but subsequent code pushes will not cause anything.

This could be useful for developers that want to cause all code pushes on a PR to be deployed using [samson] in the PR description or simply deploy once using a PR comment with [samson]

I'm open to either way, but as it stands I left the behavior as is described here.

@mwerner
Copy link
Contributor Author

mwerner commented Feb 29, 2016

Was discussing the usage of [samson]. Should we use something that prevents us from accidentally hitting the webhook on a markdown comment mentioning samson?

@steved
Copy link
Contributor

steved commented Feb 29, 2016

charon does (^|\s)\[charon review\]($|\s) as the regex match

@steved
Copy link
Contributor

steved commented Feb 29, 2016

wfm 👍

@@ -29,6 +30,19 @@
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.

@mwerner
Copy link
Contributor Author

mwerner commented Feb 29, 2016

Decided to follow Charon's pattern using [samson review] I didn't want to use "deploy" or "publish" since those are project specific terms. I'm good with "review" and this will keep things consistent.

mwerner added a commit that referenced this pull request Feb 29, 2016
Enable Webhooks for Pull Requests
@mwerner mwerner merged commit 306589a into master Feb 29, 2016
@jonmoter jonmoter deleted the matt/webhooks branch April 24, 2016 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants