-
Notifications
You must be signed in to change notification settings - Fork 291
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
Convert Slack Communication from RTM to WebSocket #693
Conversation
51ea505
to
cb99b8b
Compare
Will rebase and resolve conflicts after I got approvals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments, but they're not blockers.
The comment regarding websockets was a left as a pure "how does this work now" question.
Thx
@@ -0,0 +1,21 @@ | |||
package utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, why did you think we should extract the dumper into its own file?
It's only used in the context of E2E tests and occupied 7 lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I have used it in also debug logs in slack response messages. builtin formatters are not good on printing nested fields all the time.
}() | ||
|
||
for { | ||
select { | ||
case <-ctx.Done(): | ||
b.log.Info("Shutdown requested. Finishing...") | ||
return rtm.Disconnect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a quick look around to understand how disconnects work in the context of websockets.
For my understanding only ... to confirm:
1 - disconnects are not predictable but they are handled by the Slack library.
2 - When we terminate BotKube we're assuming the server will terminate the connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good catch, please see my updated impl. I have used RunContext(ctx) instead of Run(), so that once ctx is done, it will also close connection on websocket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easy to miss. I was trying to make sense of it myself having never used the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of code review, LGTM.
I'll need to manually test, prob after 0.13.0 release.
fbd87dc
to
93309b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to let you know that I tested that as a part of #715 with own Socket Bot and it worked well 👍
I left really minor comments and I leave approving this PR for the first reviewer because I didn't run the e2e test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Confirmed that Helm E2E tests ran successfully.
Description
Changes proposed in this pull request:
Related issue(s)
Fixes #631
Testing
Note: Since socketmode should be enabled on the slack app, I created new app (BotKubeV2) in CI Workspace, you can see it here. Please refer app page to get bot token and app token. bot token is already visible as you are familiar with current notation. For app level token, scroll down on app overview page and generate token as shown below.
The integration tests fails as it uses outdated settings from main. I tested that locally:
1. Environment setup
2. Install Botkube
3. Test
4. Check Logs
You can verify test results in our official CI workspace here