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 webex as destination #5574

Merged
merged 25 commits into from
Sep 5, 2023

Conversation

anirudhbagri
Copy link
Contributor

What type of PR is this? (check all applicable)

  • New Alert Destination - Webex

Description

Added Webex as an alert destination

Related Tickets & Documents

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

@anirudhbagri anirudhbagri mentioned this pull request Aug 23, 2021
1 task
@guidopetri
Copy link
Contributor

@anirudhbagri , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

@justinclift
Copy link
Member

After fixing the merge conflict through the web interface, it was failing the formatting check test.

When fixing that though (make format), it was unable to complete fully due to other errors:

redash/destinations/webex.py:5:1: F403 'from redash.destinations import *' used; unable to detect undefined names
redash/destinations/webex.py:8:13: F405 'BaseDestination' may be undefined, or defined from star imports: redash.destinations
redash/destinations/webex.py:145:1: F405 'register' may be undefined, or defined from star imports: redash.destinations

Those would need to be fixed (by @anirudhbagri probably?), then this can be merged. 😄

@guidopetri guidopetri self-assigned this Aug 25, 2023
@guidopetri
Copy link
Contributor

#5230 had some changes that are relevant to this PR as well, and it seems we don't have any tests on this branch yet. @anirudhbagri any interest in continuing development here? I saw you merged the master branch in about a week ago, so I don't want to step on your toes. :)

client/app/assets/images/destinations/webex.png Outdated Show resolved Hide resolved
redash/destinations/webex.py Show resolved Hide resolved
redash/destinations/webex.py Outdated Show resolved Hide resolved
redash/destinations/webex.py Outdated Show resolved Hide resolved
redash/destinations/webex.py Outdated Show resolved Hide resolved
@guidopetri
Copy link
Contributor

Missing a test now, but otherwise looks good.

@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Merging #5574 (67428d2) into master (710dd8c) will increase coverage by 0.06%.
Report is 1 commits behind head on master.
The diff coverage is 84.09%.

@@            Coverage Diff             @@
##           master    #5574      +/-   ##
==========================================
+ Coverage   60.87%   60.94%   +0.06%     
==========================================
  Files         155      156       +1     
  Lines       12718    12758      +40     
  Branches     1729     1735       +6     
==========================================
+ Hits         7742     7775      +33     
- Misses       4747     4752       +5     
- Partials      229      231       +2     
Files Changed Coverage Δ
redash/settings/__init__.py 98.65% <ø> (ø)
redash/destinations/webex.py 84.09% <84.09%> (ø)

... and 1 file with indirect coverage changes

@justinclift
Copy link
Member

Cool. Should we merge this now, or are you still wanting to do stuff with it? 😄

@guidopetri
Copy link
Contributor

Still adding a test ;) it should pass now (it did on my local), so we can merge unless you think otherwise.

@justinclift
Copy link
Member

Heh Heh Heh. I'll leave it alone and let you merge it when you're happy with it. 😄

@guidopetri guidopetri enabled auto-merge (squash) September 4, 2023 23:51
@guidopetri guidopetri merged commit c2e7df0 into getredash:master Sep 5, 2023
13 checks passed
@guidopetri
Copy link
Contributor

@anirudhbagri thanks again for your contribution :)

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.

3 participants