-
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
Update Cloud Slack to always respond in a thread #1372
Conversation
52af056
to
f9d91ff
Compare
52c539f
to
85cf281
Compare
85cf281
to
df16e04
Compare
@@ -1009,20 +998,17 @@ func runBotTest(t *testing.T, | |||
}, | |||
} | |||
t.Log("Expecting bot message in second channel...") | |||
err = botDriver.WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.SecondChannel().ID(), 2, secondCMUpdate) | |||
err = botDriver.OnChannel().WaitForMessagePostedWithAttachment(botDriver.BotUserID(), botDriver.SecondChannel().ID(), 2, secondCMUpdate) | |||
assert.NoError(t, err) |
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.
it was missing 😶
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.
LGTM, few small comments
@@ -12,6 +12,11 @@ inputs: | |||
runs: | |||
using: "composite" | |||
steps: | |||
- name: Setup Go | |||
uses: actions/setup-go@v4 |
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.
uses: actions/setup-go@v4 | |
uses: actions/setup-go@v5 |
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.
will do in the next PR globally to all our actions to not trigger the CI once again on this PR 👍
t.Log("Waiting for confirmation message...") | ||
expectedClusterDefaultMsg := fmt.Sprintf(":white_check_mark: Instance %s was successfully selected as the default cluster for this channel.", deployment.Name) | ||
err = tester.WaitForLastMessageEqual(tester.BotUserID(), channel.ID(), expectedClusterDefaultMsg) | ||
if err != nil { // the new cloud backend not release yet |
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.
We can remove the fallback later after release 🤔 Can you make a reminder for it? Thanks!
@@ -600,6 +600,24 @@ func (b *CloudSlack) send(ctx context.Context, event slackMessage, resp interact | |||
return nil | |||
} | |||
|
|||
func (b *CloudSlack) uploadFileToSlack(ctx context.Context, event slackMessage, resp interactive.CoreMessage) (*slack.File, error) { |
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.
Why can't we use the uploadFileToSlack
from other Slacks? From what I can see it is 1:1 - instead of copying the logic you could run that function 🤔
BTW it's in slack_legacy.go
but we can move it to slack_shared.go
.
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.
this function is using new event.GetTimestamp()
func so the messages are posted in the thread, while on socket slack it will still land on the channel level 👍
Description
Changes proposed in this pull request:
Testing
Done by CI. However, you can manually install Cloud Slack and test it e.g. interactivity, file upload, etc.
CI example: https://github.com/kubeshop/botkube/actions/runs/7817153682/job/21329595326
Related issue(s)
Fix https://github.com/kubeshop/botkube-cloud/issues/862