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

Added check slack channel in config, when get message #39

Merged
merged 1 commit into from
Feb 25, 2019
Merged

Added check slack channel in config, when get message #39

merged 1 commit into from
Feb 25, 2019

Conversation

gimmetm
Copy link
Contributor

@gimmetm gimmetm commented Feb 20, 2019

For cluster-channel pair use

@PrasadG193
Copy link
Collaborator

Hey @gimmetm,
Thanks for the PR 🙂

Looks like hack/verify-gofmt.sh is failing.

$ hack/verify-gofmt.sh
diff -u ./pkg/slack/slack.go.orig ./pkg/slack/slack.go
--- ./pkg/slack/slack.go.orig	2019-02-20 05:50:34.363532422 +0000
+++ ./pkg/slack/slack.go	2019-02-20 05:50:34.363532422 +0000
@@ -75,7 +75,7 @@
 				}
 				// if checkChannel is true
 				// Serve only if current channel is in config
-				if b.CheckChannel && (b.ChannelName != channel.Name){
+				if b.CheckChannel && (b.ChannelName != channel.Name) {
 					continue
 				}
 
@@ -90,7 +90,7 @@
 					}
 					// if checkChannel is true
 					// Serve only if current group is in config
-					if b.CheckChannel && (b.ChannelName != group.Name){
+					if b.CheckChannel && (b.ChannelName != group.Name) {
 						continue
 					}
 				}
Run ./hack/update-gofmt.sh
The command "hack/verify-gofmt.sh" failed and exited with 1 during .

Running hack/update-gofmt.sh should fix that.

@PrasadG193
Copy link
Collaborator

  # Channels configuration
  communications:
    slack:
      channel: 'SLACK_CHANNEL'

@gimmetm, the communication.slack.channel in the configuration is only for restricting push notifications from BotKube. So as per the design, we should be able to execute kubectl commands from any channel, group (shared chat) or even from Direct Message to BotKube.
I am trying to understand what use case you are trying to solve?

@PrasadG193
Copy link
Collaborator

BTW we are disabling notifier commands execution outside the channel as a part of separate issue

@gimmetm
Copy link
Contributor Author

gimmetm commented Feb 20, 2019 via email

if err == nil {
// Message posted in a group
// Serve only if starts with mention
if !strings.HasPrefix(ev.Text, "<@"+botID+"> ") {
continue
}
// if checkChannel is true
// Serve only if current group is in config
if b.CheckChannel && (b.ChannelName != group.Name) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gimmetm can you please remove this check? We should be able to execute BotKube commands from group chats.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PrasadG193 This check is for private channel. (https://api.slack.com/methods/groups.info)
group chat info might be in conversation.info or else.

@PrasadG193
Copy link
Collaborator

Also, please squash your commits into one

…annel pair use)

Replace slack api(channel.info, gruop.info) -> conversation.info
@PrasadG193
Copy link
Collaborator

PrasadG193 commented Feb 21, 2019

@gimmetm actually, client.GroupInfo() (https://godoc.org/github.com/nlopes/slack#Client.GetGroupInfo) returns true for a private channel as well as group chat. We need to find a better way to differentiate between them.

@gimmetm
Copy link
Contributor Author

gimmetm commented Feb 21, 2019

@PrasadG193 I replaced GetChannelInfo&GetGroupInfo -> GetConversationInfo,
and checked (IsChannel, IsPrivate)
Please review it.

@PrasadG193
Copy link
Collaborator

Nice! But still, group BotKube is not responding in group chat. Can you please test it on your setup?

@PrasadG193
Copy link
Collaborator

@gimmetm actually, client.GroupInfo() (https://godoc.org/github.com/nlopes/slack#Client.GetGroupInfo) returns true for a private channel as well as group chat. We need to find a better way to differentiate between them.

Let's track this as a separate issue

@PrasadG193 PrasadG193 self-requested a review February 25, 2019 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants