[Question] Still necessary to use a fork? #51
Comments
|
@geekdave My personal fork, https://github.com/mbland/hubot-slack-github-issues, depends on the latest official https://github.com/slackhq/hubot-slack. You can depend on that fork in the very short-term, or wait until I rebrand and repackage it as mbland/slack-github-issues, which I may get to even today. (Code's been done for a while; I've been obsessing over a packaging detail.) |
|
Thanks for the tip, @mbland ! Here's the relevant snippet of my bot's I created a github bot user account and generated a token, then set it as follows: My test repo is http://github.com/TestArmada/discuss, so I set up the following config: After pushing and deploying, and adding the evergreen tree emoji I see this in my Heroku logs. Any idea where I may have gone wrong? |
|
What version of Node are you using? |
|
Wow, good catch. Looks like the slack hubot yeoman generator pinned my bot to And the bot responds: |
|
I have no idea where that is coming from. Do you have a repo to share, and more of the log file? |
|
Sure! Here's the repo: https://github.com/TestArmada/captainhook And here's the full log: |
|
Are you actually not able to file issues? It works on my machine... 😏 Does that If you haven't tried to file an issue yet, it could well be another plugin that's causing the error. You could try pulling things out of I cloned your repo, updated the config for my user/repo, and ran it locally (not on Heroku) with my credentials under Node v6.2.1, and sure enough got it to work (after a first attempt pointed out that I hadn't enabled issues on my repo yet):
The log from the above (where I've redacted the message ID as The Believe it or not, I've never messed with Heroku (yet), so if the problem is there, I'd have to do some more digging to help you. |
|
Weird! I ran it locally and got the same issue. I wonder if I've set up a token wrong or something? I'll keep working on it. Really appreciate you taking a look! |
|
I'm curious, though: Did you try tagging a message with an emoji (presuming the 🌲)? So it's running, but the issue isn't getting filed and you're seeing that error? Or are you seeing the error without tagging a message? If you do then tag a message, what happens? I'm asking because based on when you said earlier that "And the bot responds:", I'm not clear if that meant you actually tried tagging a message yet or not. |
|
Yep, the error is only at the point of adding the evergreen emoji. Here are the logs with hubot verbose logging:
|
|
I swear, that looks like you're getting an older version of Either way it really seems like something's out of sync. Any chance you can do a fresh |
|
Same result with a fresh clone, with When I |
|
Whoops, I just need to update the README. OK, just re-ran with Still looking, though I gotta run soon... The first message you should see from the plugin is |
|
Oh, I think I just realized something...somehow Is there anything special about the channel you're tagging a message in? Have you invited your Hubot to join it (though I guess it wouldn't reply if it wasn't)? What if you did it from I guess what I'm saying is, there may be a feature of your actual Slack channel that's different from mine. And I may need to add a new condition, test case, and a little bit of logging based on what we find out. |
|
🎉 🎉 🎉 YES!!! 🎉 🎉 🎉 That was the issue. The tagging was happening inside a private channel (of which the bot was a member). When I switched to a public channel, it worked like a charm. Thanks so much for helping to debug. |
|
Hmm, OK. Good to know. Going to open an issue in my fork and address it there. Oh, and woo-hoo! 🎉 Glad I could get you moving. 😃 |
As discovered in 18F#51, the `UnhandledPromiseRejectionWarning` and `PromiseRejectionHandledWarning` warnings were apparently added in v6.6.0 (https://nodejs.org/en/blog/release/v6.6.0/); specifically it was added by nodejs/node#8223. See also: nodejs/node#6439 nodejs/node#8217 https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_unhandledrejection https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_rejectionhandled https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_warning Test failures from `test/integration-test.js` after upgrading to Node v6.9.1 showed extra output such as: ``` (node:39696) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): null (node:39696) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 2) ``` This was happening because `scripts/slack-github-issues.js` created a Hubot listener that returned a `Promise` so that the integration test could use `.should.be.rejectedWith` assertions. Rather than jump through hoops to quash the warnings, this change now causes the listener's `catch` handler to return the result of the rejected `Promise`, converting it to a fulfilled `Promise` in the process. Since Hubot never used the result anyway, the only effect it has in production is to eliminate the warning messages. The tests now just check that the `Promise` returned by the listener callback is fulfilled with the expected error result, with practically no loss in clarity.
|
@mbland If you'll PR from your fork back over here, I'll get it merged and updated on npm! UpdateOn second thought, I'll update this to be a wrapper around your slack-github-issues module, similar to what you did with your fork. 👍 |
|
@mgwalker Actually, if you don't mind, I'd like to take sole ownership of the |
|
That also sounds good to me. Is there anything we need to do in npm so you can publish it? |
|
Not really, I don't think. I still have publishing access to the NPM, so I can push a new version with the GitHub repo pointing to my fork. I'll give it a try in a few minutes here. |
|
OK, done. That was easy. |
|
Fantastic, thanks! I'll finish updating our hubot. I've also put in a PR to update our readme to point to your repo instead. 😃 |
Backported from mbland/hubot-slack-github-issues#7. As discovered in 18F/hubot-slack-github-issues#51, the `UnhandledPromiseRejectionWarning` and `PromiseRejectionHandledWarning` warnings were apparently added in v6.6.0 (https://nodejs.org/en/blog/release/v6.6.0/); specifically it was added by nodejs/node#8223. See also: nodejs/node#6439 nodejs/node#8217 https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_unhandledrejection https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_rejectionhandled https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_warning A test failure from `solutions/06-integration/test/integration-test.js` after upgrading to Node v6.9.1 showed output such as: ``` - "(node:87412) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 14): Error: failed to create a GitHub issue in 18F/handbook: received 500 response from GitHub API: {\"message\":\"test failure\"}\n" ``` This was happening because `scripts/slack-github-issues.js` ignored the return value from `Middleware.execute()`, whether it was undefined or a `Promise`. For consistency's sake (and to provide a clearer upgrade path to the current state of mbland/slack-github-issues), `Middleware.execute()` always returns a `Promise`, and `scripts/slack-github-issues.js` handles and ignores any rejected Promises. This fixed the `integration-test.js` error, but also required minor updates to `solutions/{05-middleware,complete}/test/middleware-test.js`. The next commit will update the tutorial narrative to account for this change.
Regarding:
I notice that reaction support seems to be merged into hubot-slack: slackapi/hubot-slack#360
Is it still necessary for this project to use a fork of hubot-slack?
The text was updated successfully, but these errors were encountered: