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

Prepare OnCall for Unified Slack App #4232

Merged
merged 65 commits into from
Jun 3, 2024

Conversation

Konstantinov-Innokentii
Copy link
Member

@Konstantinov-Innokentii Konstantinov-Innokentii commented Apr 16, 2024

This PR does a bunch of changes to prepare OnCall for Unified Slack App:

  1. Install Slack via Chatops-Proxy. This change contains two parts: getting a Slack install link from chatops-proxy (code) and receiving a "slack installed" event from chatops-proxy (code). Also it means that OnCall doesn't need to register slack_links anymore when slack is connected/disconnected. These changes are behind UNIFIED_SLACK_APP_ENABLED flag and should be no-op if flag is not enabled.
  2. Get rid of Multiregionatily restrictions - instrument all slack interactions with a ProxyMeta - json data telling chatops-proxy where to route the interaction. Note, that it doesn't apply for "Add to resolution notes" message action - it will be handled differently in following PR.
  3. Move all chatops-proxy related stuff from common/oncall-gateway to apps/chatops-proxy

Minor changes:

  1. Remove usage of CHATOPS_V3 flag. Chatops v3 is already released (It's a refactoring from previous quarter)

engine/apps/slack/oauth.py Fixed Show fixed Hide fixed
Move slack installation & uninstallation code to separate functions to reuse betwen social-auth and chatops-proxy
Copy link
Contributor

@matiasb matiasb left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! 👍
LGTM

"payload": {
# It's not actual payload we are getting from slack, just a placeholder
"slack_id"
"some_slack_id"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this sample data ok ^?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for noticing. missed a coma. Anyway, I went ahead and used a more accurate payload example from slack docs instead of placeholder

@Konstantinov-Innokentii
Copy link
Member Author

Thanks for the updates! 👍 LGTM

Thanks! Added more tests covering our slack installation.

@Konstantinov-Innokentii Konstantinov-Innokentii added this pull request to the merge queue Jun 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 3, 2024
@Konstantinov-Innokentii Konstantinov-Innokentii added this pull request to the merge queue Jun 3, 2024
Merged via the queue into dev with commit 17f448c Jun 3, 2024
21 checks passed
@Konstantinov-Innokentii Konstantinov-Innokentii deleted the handle_chatops_proxy_broadcast branch June 3, 2024 09:14
brojd pushed a commit that referenced this pull request Sep 18, 2024
This PR does a bunch of changes to prepare OnCall for Unified Slack App:
1. Install Slack via Chatops-Proxy. This change contains two parts:
getting a Slack install link from chatops-proxy
([code](https://github.com/grafana/oncall/pull/4232/files#diff-437a77d49fc04b92d315651b3df5991000b1ab74cf60aabb21aa77cb2823bf52R46))
and receiving a "slack installed" event from chatops-proxy
([code](https://github.com/grafana/oncall/pull/4232/files#diff-976d106f0962be5c1de5e35582193f68435ed0c17f2defd6bd2857bf6e27f65d)).
Also it means that OnCall doesn't need to register slack_links anymore
when slack is connected/disconnected. These changes are behind
UNIFIED_SLACK_APP_ENABLED flag and should be no-op if flag is not
enabled.
2. Get rid of Multiregionatily restrictions - instrument all slack
interactions with a ProxyMeta - json data telling chatops-proxy where to
route the interaction. Note, that it doesn't apply for "Add to
resolution notes" message action - it will be handled differently in
following PR.
3. Move all chatops-proxy related stuff from common/oncall-gateway to
apps/chatops-proxy

Minor changes:
1. Remove usage of **CHATOPS_V3** flag. Chatops v3 is already released
(It's a refactoring from previous quarter)

---------

Co-authored-by: Vadim Stepanov <vadimkerr@gmail.com>
Co-authored-by: Rares Mardare <rares.mardare@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates release:ignore PR will not be added to release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants