-
Notifications
You must be signed in to change notification settings - Fork 119
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
Extensibility #3
Comments
An evented approach sounds excellent 👍 I didn't want to over engineer this right at the get go, that's why there's not much extensibility in it yet. Would you be able to open a PR with some changes going in the direction you're describing? |
Sure thing. I'll try to work on it tonight. I see Testing is a really tough part here. I setup an org and a bot account for testing. If anyone else is testing and wants me to add another webhook endpoint, LMK. |
Yupp. Having an endpoint to request for existing PR's was useful at the beginning. It has been overly well behaved since though, so that endpoint hasn't been of much use.
Know the feeling. Seems like the procedure you're about to put into place is pretty complex :) We should absolutely invest some time in automated tests after we get this skeleton up and running, as manually testing each of these procedures when merging changes sounds like a nightmare. |
maybe we should investigate how the nodeschool bot works? |
@Fishrock123 I looked into their impl and it is a roll-your-own setup. Everything in 1 file. |
Closed by #8 |
The path I was going was to use @rvagg's github-webhook-handler which sets things up in an evented way: when hook data comes in, emit the event. I took it a step further to emit
event+'.'+action
for more granularity.I created a scripts directory where scripts could be added and they can listen to whatever events they need.
Here is echo.js that I created as an example:
How do you feel about going this route? It looks like the work in
pollTravis
could be done with this approach.But, that was just my approach... ;) What way(s) do you see extending this to include other scripts/workers?
The text was updated successfully, but these errors were encountered: