-
Notifications
You must be signed in to change notification settings - Fork 731
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
feat: Update slack_bot #1353
base: master
Are you sure you want to change the base?
feat: Update slack_bot #1353
Conversation
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.
PR naming is not following our standard, pre-commit is not passing
Thanks @Wendong-Fan, The related issue has been fixed. |
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.
Thanks @Asher-hss , just curious, I guess you created a new slack socket mode bot, but in the new socket mode bot, you did the work of being compatible with socket mode and http mode. Why not just do what the socket mode should do in the slack socket mode bot?
camel/bots/slack/slack_app.py
Outdated
scopes: Optional[str] = None, | ||
signing_secret: Optional[str] = None, | ||
client_id: Optional[str] = None, | ||
client_secret: Optional[str] = None, | ||
redirect_uri_path: str = "/slack/oauth_redirect", | ||
installation_store: Optional[AsyncInstallationStore] = None, | ||
socket_mode: bool = True, |
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.
Hi @Asher-hss , when we create a new slack app, socket mode is turned off by default. Let's set socket_mode to False
to maintain consistency with the official configuration.
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.
sure
self._handler: Optional[ | ||
Union[AsyncSlackRequestHandler, AsyncSocketModeHandler] | ||
] = None | ||
if not self.socket_mode: |
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.
Hi @Asher-hss , the slack app contains OAuth verification and Message communication. In socket mode, the OAuth process also needs to be allowed. I don't think the verification here is appropriate.
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.
The purpose here is:
- Different modes correspond to different validation procedures to avoid confusion in validation.
- Use default validation in socket mode to reduce the learning curve for users.
Hi Coco,I didn't understand what you meant. Could you explain it more clearly? |
pre commit, conflict with master branch, pytest need to be fixed @Asher-hss |
Description
close #slack
Motivation and Context
close #slack
Types of changes
What types of changes does your code introduce? Put an
x
in all the boxes that apply:Implemented Tasks
Checklist
Go over all the following points, and put an
x
in all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!