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

Allow web-hooks to be configured to fire on test or deploy #6

Open
wants to merge 86 commits into
base: master
Choose a base branch
from

Conversation

ProZachJ
Copy link
Contributor

Fixes #5

I figured out why my web-hook wasn't firing after deploy. The worker was only listening for job.status.tested. So I added a listener for job.status.deployed. Instead of reusing test_exitcode I add a new property to to the payload deploy_exitcode. I also changed the output to say which webhook listener is being fired.

I tested this on my fully updated strider. However I had to remove strider-sauce to work around #4

@ProZachJ
Copy link
Contributor Author

screen shot 2014-08-09 at 8 44 54 pm

@jaredly
Copy link
Member

jaredly commented Aug 10, 2014

This fires all webhooks for both events. Could you add something to the config page to allow the user to specify whether a hook should fire on test or on deploy?

@ProZachJ
Copy link
Contributor Author

I'll take a look at the UI code and see what I can do.

Should I add a hook for each phase (env, prep, test, deploy, cleanup) and then a check box grid for each web-hook?

@jaredly
Copy link
Member

jaredly commented Aug 10, 2014

Let's go for simple first - just test and deploy. If people find the need
for more than that, we can add it later. I think I'd also prefer to have 1
action per webhook, and let people make multiple hooks if they want
multiple actions.

On Sat, Aug 9, 2014 at 7:25 PM, ProZachJ [email protected] wrote:

I'll take a look at the UI code and see what I can do.

Should I add a hook for each phase (env, prep, test, deploy, cleanup) and
then a check box grid for each web-hook?


Reply to this email directly or view it on GitHub
#6 (comment)
.

@ProZachJ
Copy link
Contributor Author

Ok try this out, as far as I've tested manually everything is good to go.

Oh also...we wrote a hubot script to listen for the web-hook and post it into any chatroom that your specify in the callback URI.

https://github.com/mattjay/hubot-strider

@jaredly
Copy link
Member

jaredly commented Aug 13, 2014

Awesome :) Looks like there are some merge conflicts --- could you merge with current master?

@ProZachJ
Copy link
Contributor Author

Done

context.comment('Failed to prepare webhook payload: ' + e.message);
}
}
else if(hook.trigger === 'deploy'){
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't belong in onTested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is there to register the deploy listeners after the tests pass. If you register the deploy listener when the test listeners are registered and tests fail you will orphan the listener and it will fire two web-hooks on the next deploy. I guess I could check the payload to see if tests failed to remove the listeners but this seemed cleaner.

I'll see if I can think of a way to refactor.

function onTested(id, data) {
io.removeListener('job.status.tested', onTested)
io.on('job.status.tested', onTested)
if(job.type === 'TEST_AND_DEPLOY'){
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only register the deploy listener if the job is going to deploy.

@ProZachJ
Copy link
Contributor Author

Comments added

@ProZachJ
Copy link
Contributor Author

ProZachJ commented Dec 4, 2014

Any chance this can get merged? I'm having to pull strider from my fork when I want to deploy.

@knownasilya
Copy link
Member

@ProZachJ If you could rebase and squash your commits, I'd be willing to merge.

@ProZachJ
Copy link
Contributor Author

Will do.

On May 13, 2015, at 9:47 AM, Ilya Radchenko [email protected] wrote:

@ProZachJ If you could rebase and squash your commits, I'd be willing to merge.


Reply to this email directly or view it on GitHub.

@knownasilya
Copy link
Member

Ping 👍

@ProZachJ
Copy link
Contributor Author

ProZachJ commented Jun 2, 2016

Wow I totally forgot to do this after I got it working on my server. I'll
take a look tonight.

On Jun 2, 2016 7:41 AM, "Ilya Radchenko" [email protected] wrote:

Ping 👍


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ACTWOglG5GlgzaL9dO8b7T1TuB6agZuLks5qHs9sgaJpZM4CV0gA
.

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.

Webhook does not fire in correct order.
3 participants