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

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

Closed
johnseekins-pathccm opened this issue Apr 3, 2024 · 6 comments · Fixed by #224
Closed

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

johnseekins-pathccm opened this issue Apr 3, 2024 · 6 comments · Fixed by #224
Labels

Comments

@johnseekins-pathccm
Copy link

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
Copy link
Member

stephenyeargin commented Apr 4, 2024

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
Copy link
Author

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
Copy link
Member

stephenyeargin commented Apr 4, 2024

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.

robot.respond(/(pager|major)( me)? incident (.*)$/i, function (msg) {
msg.finish();

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
Copy link
Member

@johnseekins-pathccm Try out 4fc0cdf

@johnseekins-pathccm
Copy link
Author

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
Copy link
Member

Released with v4.0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants