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

emit github events #8

Merged
merged 2 commits into from
Apr 8, 2016
Merged

emit github events #8

merged 2 commits into from
Apr 8, 2016

Conversation

williamkapke
Copy link
Contributor

Not tested- but should mostly be correct ;)

EDIT: Testing & working!

I'm using a .env file like discussed in #7 (comment)

You'll probably notice that github-webhook-handler is referencing my fork- hopefully @rvagg will accept my PR and that can be replaced soon. If not, I can do it a uglier way. EDIT: Needed to go a different direction in the end anyhow.

.DS_Store
.env

This comment was marked as off-topic.

@williamkapke
Copy link
Contributor Author

This just ran this successfully here: https://github.com/no-duh/nodejs.org/pull/1

Travis updated the PR status also... I'm guessing it is because I gave it permission to and the https://github.com/node/nodejs.org repo doesn't give Travis the permission to..?

I tried to follow along nodejs/nodejs.org#355 but didn't grasp what is going on. I'd like to completely mirror the behavior if I can.

@williamkapke williamkapke changed the title [WIP] emit github events emit github events Apr 7, 2016
@Fishrock123
Copy link
Contributor

@williamkapke Try doing it in my org, I can set the permissions up.

You need to enable advanced restrictions, try to sync travis, ask for org permission from your own permissions, turn travis on after you've approved it, and then revoke the permission from the org.

@williamkapke
Copy link
Contributor Author

Ok- as you probably saw, I created the nodejs.org repo there, but then deleted it. I didn't use it because it added everyone as watchers and I wanted to be considerate about not spamming. I'll set it up now that I know it is working and the notifications shouldn't be as bad.

@williamkapke
Copy link
Contributor Author

@Fishrock123 ok, I guess you'll need to setup Travis. It won't let me do it...

screenshot 2016-04-06 20 21 31

app.on('pull_request.opened', (event) => {
const owner = event.repository.owner.login
const repo = event.repository.name
if (repo !== 'nodejs.org') return

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@Fishrock123
Copy link
Contributor

@phillipj
Copy link
Member

phillipj commented Apr 7, 2016

@williamkapke merged #12 to get enable the bot on the readable-stream repo, the merge conflict should be trivial to resolve. I renamed pollTravis.pollAndComment() -> pollTravis.pollThenComment().

By rebasing on top of master you'll even get Travis to run this PR ;)

Please confirm this is good to go after the conflict has been resolved, and I'll be more than happy to push it to production.

@Fishrock123
Copy link
Contributor

pollThenComment should probably renamed to pollThenStatus here I guess?

@williamkapke
Copy link
Contributor Author

Rebased. Tested. Here's the log output...

event@TestOrgPleaseIgnore/nodejs.org: pull_request.opened
Thu, 07 Apr 2016 19:48:28 GMT display_travis_status /TestOrgPleaseIgnore/nodejs.org/pull/2 opened
! TestOrgPleaseIgnore/nodejs.org/#2 Was not able to find matching build, will do check #2 in 30 seconds
* TestOrgPleaseIgnore/nodejs.org/#2 "started" build found, will do check #3 in 30 seconds
* TestOrgPleaseIgnore/nodejs.org/#2 "started" build found, will do check #4 in 30 seconds
* TestOrgPleaseIgnore/nodejs.org/#2 Github comment created
event@TestOrgPleaseIgnore/nodejs.org: issue_comment.created

See:
https://github.com/TestOrgPleaseIgnore/nodejs.org/pull/2

@phillipj
Copy link
Member

phillipj commented Apr 7, 2016

pollThenComment should probably renamed to pollThenStatus here I guess?

The latter sounds like we're using the Status API, but we're still commenting on PR's here.. Or did I misunderstand?

@Fishrock123
Copy link
Contributor

The status here was emitted by travis itself, right?

@phillipj
Copy link
Member

phillipj commented Apr 8, 2016

LGTM

Looks like it introduces secrets, so we'll have to update the active PRs before pushing it to prod

@williamkapke
Copy link
Contributor Author

Ah yes. I added the header validation. So the GITHUB_WEBHOOK_SECRET will need to be added.

@williamkapke williamkapke merged commit b54189d into nodejs:master Apr 8, 2016
app.on('pull_request.opened', (event) => {
const owner = event.repository.owner.login
const repo = event.repository.name
if (!~enabledRepos.indexOf(repo)) return

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@Fishrock123 Fishrock123 mentioned this pull request Apr 10, 2016
@williamkapke williamkapke mentioned this pull request Apr 10, 2016
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