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.

Sign up for GitHub

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

Jump to bottom

Feature/Add Discord Alert Destination #6106

Merged
merged 15 commits into from
Jul 21, 2023
Merged

Feature/Add Discord Alert Destination #6106

merged 15 commits into from
Jul 21, 2023

Conversation

VitalyVakhteev
Copy link
Contributor

@VitalyVakhteev VitalyVakhteev commented Jun 21, 2023 •
edited
Loading

What type of PR is this?

  • Feature

Description

Added a new Discord destination that functions similarly to Slack. Asks for a URL (from a webhook) and a name for the destination.

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Add a query, add the destination with your own webhook URL, add an alert, and point the alert at the Discord destination.

Related Tickets & Documents

N/A

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

image

@justinclift
Copy link
Member

@VitalyVakhteev Note that you're not being ignored. We're just getting some core stuff with the getredash/redash repo figured out first. 😄

@VitalyVakhteev
Copy link
Contributor Author

Yeah, just wanted to make sure the feature gets merged into both repositories.

@justinclift
Copy link
Member

justinclift commented Jun 29, 2023 •
edited
Loading

I've just rebased this onto the current tip of the getredash/master branch, mainly so it redoes the whole CI part of things.

I'm wanting to fix a bunch of dependencies before we merge this due to the boto3 version upgrade it includes. So, it might need to wait a few days until I have time to get that finished first.

@justinclift
Copy link
Member

@wlach Is this the kind of thing you'd be interested in reviewing? 😄

@justinclift
Copy link
Member

justinclift commented Jul 10, 2023 •
edited
Loading

Just manually re-created the "Boto3 dependency" commit, as that dependency is in a different file now.

Once the CI checks pass (in maybe 1/2 hour), then this should be in a good position to review and merge. 😄

junnplus
junnplus requested changes Jul 10, 2023
View reviewed changes
redash/destinations/discord.py Outdated Show resolved Hide resolved
redash/destinations/discord.py Show resolved Hide resolved
redash/destinations/discord.py Outdated Show resolved Hide resolved
redash/destinations/discord.py Outdated Show resolved Hide resolved
redash/destinations/discord.py Outdated Show resolved Hide resolved
@junnplus
Copy link
Member

image

The icon of Discord looks out of place.

gaecoli
gaecoli reviewed Jul 11, 2023
View reviewed changes
redash/destinations/discord.py Outdated Show resolved Hide resolved
redash/destinations/discord.py Outdated Show resolved Hide resolved
redash/destinations/discord.py Outdated Show resolved Hide resolved
@justinclift
Copy link
Member

Looks like botocore will probably need to be updated at the same time as boto3 too.

That could be doable, as we've recently been updating a bunch of other dependencies. So whatever was holding back boto3/botocore before might have gone away. No idea either way personally though. 😉

VitalyVakhteev and others added 2 commits July 10, 2023 21:29
@VitalyVakhteev
Add suggestions
6362274
@VitalyVakhteev @junnplus
Apply suggestions from code review
482c077 
Co-authored-by: Jun <junnplus@gmail.com>
@VitalyVakhteev
Copy link
Contributor Author

VitalyVakhteev commented Jul 11, 2023 •
edited
Loading

image The icon of Discord looks out of place.

fixed_size

Lowered the size.

gaecoli
gaecoli reviewed Jul 11, 2023
View reviewed changes
redash/destinations/discord.py Show resolved Hide resolved
redash/destinations/discord.py Outdated Show resolved Hide resolved
gaecoli
gaecoli reviewed Jul 11, 2023
View reviewed changes
redash/destinations/discord.py Show resolved Hide resolved
redash/destinations/discord.py Outdated Show resolved Hide resolved
redash/destinations/discord.py Outdated Show resolved Hide resolved
redash/destinations/discord.py Outdated Show resolved Hide resolved
redash/destinations/discord.py Outdated Show resolved Hide resolved
redash/destinations/discord.py Outdated Show resolved Hide resolved
gaecoli
gaecoli reviewed Jul 11, 2023
View reviewed changes
redash/destinations/discord.py

colors = {
# Colors are in a Decimal format as Discord requires them to be Decimals for embeds
Alert.OK_STATE: "2600544", # Green Decimal Code
Copy link
Member

Choose a reason for hiding this comment

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

Python can read color code like green or red. So you can replace "260054" into green

Copy link
Contributor Author

@VitalyVakhteev VitalyVakhteev Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't look like that's the case, just tested it and got the following worker log:

[2023-07-11 08:58:58,920][PID:129][ERROR][root] Discord send ERROR. status_code => 400

So looks like it has to be decimal codes rather than more straightforward codes like green and red.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like that's the case, just tested it and got the following worker log:

[2023-07-11 08:58:58,920][PID:129][ERROR][root] Discord send ERROR. status_code => 400

So looks like it has to be decimal codes rather than more straightforward codes like green and red.

You can change this code in your way. Let's try it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate?

@VitalyVakhteev @junnplus
Apply suggestions from code review
81d1407 
Co-authored-by: Jun <junnplus@gmail.com>
@junnplus
Copy link
Member

Please rebase master, and fix some confilcts, run pre-commit install for format code.

@justinclift
Copy link
Member

Please rebase master, and fix some confilcts, run pre-commit install for format code.

We should probably add a make format command to do that, at some point. 😄

gaecoli
gaecoli reviewed Jul 12, 2023
View reviewed changes
requirements_all_ds.txt Outdated Show resolved Hide resolved
@wlach wlach removed their request for review July 13, 2023 19:57
@wlach
Copy link
Contributor

wlach commented Jul 13, 2023

I think I'll let the rest of y'all finish reviewing this

@junnplus
Copy link
Member

Need to resolve the conflict.

@justinclift
Copy link
Member

Fixed the merge conflict. It was just the newer boto3 and botocore deps, so super easy. 😄

@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #6106 (264a73a) into master (8376e41) will increase coverage by 0.01%.
The diff coverage is 65.62%.

@@            Coverage Diff             @@
##           master    #6106      +/-   ##
==========================================
+ Coverage   59.99%   60.00%   +0.01%     
==========================================
  Files         152      153       +1     
  Lines       12448    12480      +32     
  Branches     1687     1691       +4     
==========================================
+ Hits         7468     7489      +21     
- Misses       4771     4778       +7     
- Partials      209      213       +4
Impacted Files Coverage Δ
redash/settings/__init__.py 98.65% <ø> (ø)
redash/destinations/discord.py 65.62% <65.62%> (ø)

@justinclift justinclift enabled auto-merge (squash) July 21, 2023 12:13
@justinclift justinclift merged commit 7f4ade5 into getredash:master Jul 21, 2023
@justinclift
Copy link
Member

Yay, finally merged! 😄

EmilaineBorato pushed a commit to sondautilities/sonda_d4b_redash that referenced this pull request Jul 24, 2023
@VitalyVakhteev @junnplus @justinclift
Feature/Add Discord Alert Destination (getredash#6106)
6c1cf7c 
* Add discord webhook

* Fix icon

* Boto3 dependency

* Add unit test for Discord webhook

* Add suggestions

* Apply suggestions from code review

Co-authored-by: Jun <junnplus@gmail.com>

* Misunderstood suggestion )

* Add suggestions

* Apply suggestions from code review

Co-authored-by: Jun <junnplus@gmail.com>

* Fix test

* Fix variables in strings

* Fix formatting using our pre-commit hook

---------

Co-authored-by: Jun <junnplus@gmail.com>
Co-authored-by: Justin Clift <justin@postgresql.org>
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Jan 8, 2025
@VitalyVakhteev @junnplus
@justinclift @harveyrendell
Feature/Add Discord Alert Destination (getredash#6106)
6036d20 
* Add discord webhook

* Fix icon

* Boto3 dependency

* Add unit test for Discord webhook

* Add suggestions

* Apply suggestions from code review

Co-authored-by: Jun <junnplus@gmail.com>

* Misunderstood suggestion )

* Add suggestions

* Apply suggestions from code review

Co-authored-by: Jun <junnplus@gmail.com>

* Fix test

* Fix variables in strings

* Fix formatting using our pre-commit hook

---------

Co-authored-by: Jun <junnplus@gmail.com>
Co-authored-by: Justin Clift <justin@postgresql.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junnplus junnplus junnplus requested changes

@gaecoli gaecoli gaecoli left review comments

Assignees
No one assigned
Labels
Feature: Alerts Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@VitalyVakhteev @justinclift @junnplus @wlach @gaecoli @guidopetri