eschutho opened a new issue, #29263: URL: https://github.com/apache/superset/issues/29263
## Motivation We are facing an issue with the recent update in Slack's API. Slack has rolled out `upload_files_v2` and has discontinued access to any new workspaces on the previous version. This poses a problem as the older instances of Superset cannot be automatically updated to use this new API since there a few differences: 1) The new API accepts only one channel at a time. 2) The new API uses ids instead of names for channels 3) To fetch the channels/translate from ids to names, the Slack workspace admin must add the scope `channels:read` to their application. - We cannot leave everyone on the v1 api because new Slack workspaces will not work with the old api. - We cannot move everyone to v2 because it will be a breaking change as the admin of the workspace will be required to add a new Slack scope/permission. ## Proposed Change Develop a solution that integrates with Slack's new `upload_files_v2` API and ensure backward compatibility with older instances of Superset. **For converting channels from names to ids, these are two options:** 1) **Continue to save channel names** and always fetch the ids from the slack api in order to upload files to the new v2, provided the scope is available to use the v2 api, otherwise, use the v1 api. At some point we will need to run a migration that will convert from names to ids, but I am wary of calling a third party api during a migration, as we don’t want to be dependent on either the api’s availability or any rate-limiting that might block the migration and thus a version upgrade. 2) **Save the channels as ids as we run the reports**, creating a new `NotificationMethod` for `SlackV2` for any report that has the correct scopes in place. In this option, we don’t need to call the api every time (unless the admin hasn’t updated their Slack scopes), and it will set us up later for when the v1 api is eventually dropped, or we can remove it from Superset during a major version bump. **In this option we also have two other choices:** 1) **Save only the report that is being run.** This option is cleaner code-wise because it limits the scope of the responsibility of the report executor, but after a period of time when all reports have run and have been converted, there still may be some reports that haven’t been activated that need to be converted. 2) **Save all Slack reports when the first V1 Slack report is executed**. This would limit the need for multiple updates, and will convert any inactive reports as well, but it will add more complexity to the report command. In this POC, I am proposing that when running a `Slack` (v1) `Notification` we check to see if the scopes are available to fetch channels. If they are, we will save the ids as channels and update the `Notification` of the `ReportSchedule` to `SlackV2` . On the front end UI, I have added an auto complete async selector that calls a new API and fetches all Slack channels. It also can take a search field. Slack’s api is paginated but does not have a search option, so all channels will need to be fetched before the search can be applied. This UI selector can take either names and convert to ids or ids and convert to names. V1 channels will be names, and V2 channels will be saved as ids, but will need to be converted to names for users to understand which channels their messages are being sent to. The UI will fetch all the channels on load. If the fetch is successful, it means that the necessary scopes are in place for the v2 api, and it will create or update an existing Slack notification to V2 when edited. https://github.com/apache/superset/assets/5186919/67f87e92-da31-4c62-8ee6-9fb5ee38b4f5 During this time, we will log warnings in the Superset log stream to alert administrators to update their Slack workspaces. **Feature flag** By providing a feature flag, we can reduce the load of Slack api calls being made unnecessarily if a Slack workspace owner hasn’t updated the scopes. Because we want to give Superset admins notifications that the Slack v1 api is deprecated and inform them to update the scope, I would recommend to have this Feature Flag on by default, but give Superset admins the option to turn it off temporarily if it causes any issues with the Slack api, especially if they are getting rate limited. ## New or Changed Public Interfaces The new public interface would include an api `/report/slack_channels` that can be used to fetch all active slack channels associated with the slack token. It will take a search string, and although the slack api is paginated, I do not feel that this api needs to be paginated, as it only returns the channel id and name, as opposed to the Slack API which returns much more information about the channels. This api will also have the same permission scopes as `can_report:post` or in other words, the same permissions needed to create a report. ## New Dependencies This proposed change will make us dependent on the new Slack's `upload_files_v2` API. ## Migration Plan and Compatibility **When we drop support for Slack’s file upload v1 api, during a major Superset version bump, we have a few options:** 1) Run a migration that changes all `Slack` (v1) `Notification`s to `SlackV2` but leave the channels as names. When running the report, if we receive a `no_channel_found` error, we can attempt to fetch the channels from the Slack api and convert the names to ids. If this succeeds, we can then resave the recipient channels as ids. 2) Additionally (or alternately) once the scopes are updated, a report owner can fix the report as before by either editing the report in the UI, which will send the channel names to the api and return the ids, and upon saving, will save the ids. 3) If we want to drop the lookup when a `no_channel_found` error is received, we can leave option 2 as the required method to “fix” a broken report. The suggested approach is to look up the ids and save the channels as ids as a `SlackV2` `Notification` and also create all new reports as `SlackV2` if the scopes are available. Communicate to admins to update their scopes during this time when both Slack api versions are supported. At some breaking change, 5.0 or 6.0, run a migration that updates all `Slack` to `SlackV2`, remove the `Slack` `Notification`s and leave the lookup and auto conversions in the codebase for one more major release. At that time, remove the backend lookup and only leave the frontend lookup. ## Rejected Alternatives Options that would involve a breaking change have been rejected, such as updating all reports to v2 and requiring that all Slack workspaces have the new scope in order to continue to use the Slack reports. As mentioned, calling Slack’s channels api during a migration does not seem to be a feasible option because we cannot guarantee that the scope will be available at the time that the migration is run. Plus there can be other downtime issues with calling a third-party API during a migration. ### Considerations The new channel api `conversations_list` is also [[rate-limited with Tier2 limits](https://api.slack.com/methods/conversations.list)](https://api.slack.com/methods/conversations.list). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
