hubot-archive / hubot-pager-me

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

Can't take pager for schedule like `Incident.*` #223

Closed johnseekins-pathccm closed 7 months ago

johnseekins-pathccm commented 7 months ago

If I say @Hubot pager me Incident Commander 5, Hubot crashes:

at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
--
at endReadableNT (node:internal/streams/readable:1696:12)
at IncomingMessage.emit (node:events:530:35)
at IncomingMessage.<anonymous> (/hubot/node_modules/scoped-http-client/src/index.js:95:22)
at /hubot/node_modules/hubot-pager-me/src/pagerduty.js:60:14
at /hubot/node_modules/hubot-pager-me/src/pagerduty.js:156:7
at /hubot/node_modules/hubot-pager-me/src/scripts/pagerduty.js:108:16
at formatIncident (/hubot/node_modules/hubot-pager-me/src/scripts/pagerduty.js:1006:25)
TypeError: Cannot read properties of undefined (reading 'title')
 
^
const summary = inc.title;
/hubot/node_modules/hubot-pager-me/src/scripts/pagerduty.js:1006

This appears to be a matching issue with https://github.com/hubot-archive/hubot-pager-me/blob/main/src/scripts/pagerduty.js#L95 doing case-insensitive matching?

Any advice on fixing this would be great.

stephenyeargin commented 7 months ago

Tagging #49, because it would make it a lot easier to debug if we could put a test case around it.

So what I think is happening is that it is looking up an incident named "Commander 5". I don't think that's what you intended. Either way, it shouldn't bomb out when it can't find a matching incident. Going to knock that out.

hubot pager schedule Incident Commander 5 may be more what you're looking for.

johnseekins-pathccm commented 7 months ago

hubot pager schedule <schedule> [days] - show <schedule>'s shifts for the next x [days] (default 30 days)

vs.

hubot pager me <schedule> <minutes> - take the pager for <minutes> minutes

Those seem to do very different things.

Is the take away from this issue that this plugin can't support overriding PagerDuty schedules with names that start with "Incident"? It seems like ensuring regexes in the plugin are case sensitive would address my issue...

stephenyeargin commented 7 months ago

Ah, I track now. One reason to keep the commands case insensitive is because we've all goofed up when typing something in a hurry. What I think we can do, though, is address the inadvertent collision between the commands.

https://github.com/hubot-archive/hubot-pager-me/blob/cfd8ee29d7e90b47c1e54ba9ef145d7ee0ea4d08/src/scripts/pagerduty.js#L95-L96

This is a rather greedy regex. The only two formats an incident ID can take are \w+ or \d+, with the latter being the more likely of the two used in chat. L96 tells the processor to stop looking for anything else. Because the schedule lookup is registered after the incident lookup, it never falls through to it.

Aside from the fix in #224 to keep the bot from crashing, tightening up that line would likely resolve the issue you're seeing.

stephenyeargin commented 7 months ago

@johnseekins-pathccm Try out https://github.com/hubot-archive/hubot-pager-me/pull/224/commits/4fc0cdf4a35674e4ee37711f66c01f67b2f7a18d

johnseekins-pathccm commented 7 months ago

Looks promising:

hubot-dev> @hubot-dev pager me Incident Commander Secondary 5
{"level":20,"time":1712238728522,"pid":19547,"hostname":"John-Seekins-MacBook-Pro-16-inch-2023-","name":"hubot-dev","msg":"Message '@hubot-dev pager me Incident Commander Secondary 5' matched regex //^\\s*[@]?hubot\\-dev[:,]?\\s*(?:pager( me)? (?!schedules?\\b|overrides?\\b|my schedule\\b)(.+) (\\d+)$)/i/; listener.options = { id: null }"}
stephenyeargin commented 7 months ago

Released with v4.0.3