hubot-archive / hubot-pager-me

PagerDuty integration for Hubot
https://www.npmjs.com/package/hubot-pager-me
MIT License
73 stars 93 forks source link

Unable to trigger on a schedule #69

Closed aknapp closed 8 years ago

aknapp commented 8 years ago

After upgrading to 2.1.10, I'm unable to trigger on a schedule:

<bot> pager trigger <schedule> test

results in:

[Wed Nov 18 2015 22:06:49 GMT+0000 (UTC)] ERROR Error: 400 back from /schedules/undefined/entries
  at PagerDutyError.Error (native)
  at new PagerDutyError (<path>/node_modules/hubot-pager-me/src/pagerduty.coffee:10:1, <js>:26:51)
  at <path>/node_modules/hubot-pager-me/src/pagerduty.coffee:44:20, <js>:74:16
  at IncomingMessage.<anonymous> (<path>/node_modules/scoped-http-client/src/index.js:95:22)
  at IncomingMessage.emit (events.js:129:20)
  at _stream_readable.js:908:16
  at process._tickCallback (node.js:355:11)

[Wed Nov 18 2015 22:06:49 GMT+0000 (UTC)] ERROR TypeError: Cannot read property 'entries' of null
  at <path>/node_modules/hubot-pager-me/src/scripts/pagerduty.coffee:751:7, <js>:866:17
  at <path>/node_modules/hubot-pager-me/src/pagerduty.coffee:46:9, <js>:76:16
  at IncomingMessage.<anonymous> (<path>/node_modules/scoped-http-client/src/index.js:95:22)
  at IncomingMessage.emit (events.js:129:20)
  at _stream_readable.js:908:16
  at process._tickCallback (node.js:355:11)
aknapp commented 8 years ago

FWIW, triggering on a individual still works fine.

stephenyeargin commented 8 years ago

If you have a moment, can you see if v2.1.7 has this issue? I know @mmrobins made some minor changes in #63 that may have affected schedule matching, but I can't spot an obvious issue.

mmrobins commented 8 years ago

I'm still able to trigger on a schedule just fine in 2.1.10

matt.robinson [4:19 PM] 
pager trigger dev platform test
hubotBOT [4:19 PM] 
@matt.robinson: :pager: triggered! now assigning it to the right user...
@matt.robinson: :pager: assigned to Dev Platform!

I'll look through the stack trace to see if anything stands out, but nothing does on first glance.

aaronblythe commented 8 years ago

The line from the call stack appears to be:

if json.entries and json.entries.length > 0

If the intention is to short circuit, should '&&' be used instead of 'and'? This may just push the error, I have yet to test.

shraddhabhata commented 8 years ago

I would put console log on json and use &&

stephenyeargin commented 8 years ago

Not 100% sure, but Coffeescript's documentation indicates the two are analogous.

stephenyeargin commented 8 years ago

Fixed in #70, released in v2.1.11

aaronblythe commented 8 years ago

Sorry I read the errors in the wrong order. It looks like in some cases the schedule is an a regular Object, and in some (this one) it is an Array. I am unsure if the first object in the array is appropriate, however PR #70 gets me to successfully triggering an incident. (NOTE: in my haste i said string in the PR and commit message where I meant object)

aknapp commented 8 years ago

Awesome, thanks everyone!

aknapp commented 8 years ago

FWIW, I still get a error when I try to trigger a page based on a schedule:

[Tue Nov 24 2015 20:56:26 GMT+0000 (UTC)] ERROR TypeError: Cannot read property 'id' of undefined
  at withCurrentOncallUser (<path>/node_modules/hubot-pager-me/src/scripts/pagerduty.coffee:744:7, <js>:858:33)
  at<path>/node_modules/hubot-pager-me/src/scripts/pagerduty.coffee:725:15, <js>:828:24
  at <path>/node_modules/hubot-pager-me/src/scripts/pagerduty.coffee:689:7, <js>:775:16
  at <path>/node_modules/hubot-pager-me/src/pagerduty.coffee:138:7, <js>:185:16
  at <path>/tank/node_modules/hubot-pager-me/src/pagerduty.coffee:46:9, <js>:76:16
  at IncomingMessage.<anonymous> (<path>/node_modules/scoped-http-client/src/index.js:95:22)
  at IncomingMessage.emit (events.js:129:20)
  at _stream_readable.js:908:16
  at process._tickCallback (node.js:355:11)
aaronblythe commented 8 years ago

Out of curiosity, what do you get when you run:

hubot pager schedules

I get that same error if I don't use a schedule that matches something that is returned from the above command. Say for example:

hubot pager trigger made_up_schedule test message

Likely there could be better error handling in the code for this case.

aaronblythe commented 8 years ago

Also from the README:

HUBOT_PAGERDUTY_USER_ID - The user id of a PagerDuty user for your bot. This is only required if you want chat users to be able to trigger incidents without their own PagerDuty user

Just want to check that you have this set, because that was my original problem that I believe was manifesting as a similar error to your original post.

aknapp commented 8 years ago

Ah, good call. It works if I use the exact name of the schedule (case, etc), but not when I don't.

My schedule name is, exactly, AllOfTechops; when I try it on alloftechops, I get the error.

aknapp commented 8 years ago

Oh, I misspoke. I don't get an error when it's not exact, it just returns nothing. I suppose that is expected?

aaronblythe commented 8 years ago

Expectation from the README is that there are two lines of feedback:

technicalpickles> hubot pager trigger ops site is down
hubot> Shell: :pager: triggered! now assigning it to the right user...
hubot> Shell: :pager: assigned to ops!

I get:

Hubot> hubot pager trigger ATRotation test if trigger works
Hubot> Shell: :pager: triggered! now assigning it to the right user...
Shell: :pager: assigned to Aaron Blythe!

However I currently only have one schedule built in pagerduty. I plan to start building out more eventually, but wanted to get this working first. How many schedules do you have?

aknapp commented 8 years ago

We currently have 15 schedules. I do get that feedback if I specify the exact name of the schedule (it pages correctly), but if I use the right name but a different case, it doesn't work. Not sure if that is expected behavior.

aaronblythe commented 8 years ago

Created a pull request #71 to give better feedback when the schedule cannot be resolved. This should short circuit before the error about "Cannot read property 'id' of undefined" and allow the chat user to know what to try next.

stephenyeargin commented 8 years ago

@aknapp / @aaronblythe : #71 has shipped. This good to close?

aknapp commented 8 years ago

I think this is good to close. I've installed the newest version and it works as expected.

stephenyeargin commented 8 years ago

:+1: