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

Add Socket mode adapter as new default #108

Merged
merged 7 commits into from
Sep 7, 2021

Conversation

theothertomelliott
Copy link
Contributor

@theothertomelliott theothertomelliott commented Sep 5, 2021

Using a Classic Slack app is still supported, but results in a deprecation warning.

Socket mode is enabled by specifying the new app_token and bot_token fields of the slack config yaml instead of the Classic api_token field.

A manifest file is provided to configure a new Socket mode app with the appropriate scopes.

Fixes #84

Deprecate the RTM version of the adapter.

Fixes #84
These were for local testing
The only use is to notify users that Gort is online, which is still supported with this change.

This also removes the need to identify the bot user on startup, which is less straightforward in Socket mode.
@theothertomelliott theothertomelliott marked this pull request as ready for review September 6, 2021 03:08
}

// OnChannelMessage is called when the Slack API emits an MessageEvent for a message in a channel.
func (s *ClassicAdapter) OnChannelMessage(event *slack.MessageEvent, info *adapter.Info) *adapter.ProviderEvent {
Copy link
Member

Choose a reason for hiding this comment

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

Note for the future: these OnFOO methods are exported in the classic adapter (though they really don't need to be), and unexported in the new adapter. We might want to make them consistent eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, do we want a follow up issue for it?

Copy link
Member

Choose a reason for hiding this comment

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

Probably a good idea.

Copy link
Member

@clockworksoul clockworksoul left a comment

Choose a reason for hiding this comment

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

Did you mean to include the cli changes in this diff?

@theothertomelliott
Copy link
Contributor Author

Did you mean to include the cli changes in this diff?

Figured it would prevent us having to get those white space changes into another PR, or having to remember to revert every time we run the books until we touch those files again.

@clockworksoul clockworksoul merged commit 345da7b into develop Sep 7, 2021
@clockworksoul clockworksoul deleted the theothertomelliott/issue84 branch September 7, 2021 13:03
@clockworksoul
Copy link
Member

SGTM!

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.

2 participants