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

Catch db_engine_spec.get_function_names exceptions #9691

Merged
merged 1 commit into from
May 8, 2020

Conversation

bkyryliuk
Copy link
Member

CATEGORY

Choose one

  • Bug Fix

SUMMARY

Short term fix for the #9678
Databases api fetches all database function names, it requires the databases to be online and return the values successfully to be able to fetch the list of the databases.
If it fails sqllab is not able to get the list of the databases and becomes disfunctional

TEST PLAN

tested on the dropbox staging environment, provided bad DB url and made sure that sqllab works.

ADDITIONAL INFORMATION

  • Has associated issue:

REVIEWERS

@villebro @dpgaspar

@codecov-io
Copy link

codecov-io commented Apr 29, 2020

Codecov Report

Merging #9691 into master will decrease coverage by 12.22%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #9691       +/-   ##
===========================================
- Coverage   65.80%   53.57%   -12.23%     
===========================================
  Files         581      350      -231     
  Lines       30323    11202    -19121     
  Branches     3095     2775      -320     
===========================================
- Hits        19953     6002    -13951     
+ Misses      10189     5017     -5172     
- Partials      181      183        +2     
Flag Coverage Δ
#cypress 53.57% <ø> (?)
#javascript ?
#python ?
Impacted Files Coverage Δ
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js 0.00% <0.00%> (-93.19%) ⬇️
.../src/dashboard/components/FilterIndicatorGroup.jsx 11.76% <0.00%> (-88.24%) ⬇️
...c/explore/components/controls/withVerification.jsx 9.09% <0.00%> (-87.88%) ⬇️
...src/dashboard/components/gridComponents/Header.jsx 10.52% <0.00%> (-86.85%) ⬇️
...rc/dashboard/components/gridComponents/Divider.jsx 13.33% <0.00%> (-86.67%) ⬇️
...c/dashboard/components/gridComponents/Markdown.jsx 6.59% <0.00%> (-82.42%) ⬇️
...uperset-frontend/src/utils/getClientErrorObject.js 0.00% <0.00%> (-80.00%) ⬇️
superset-frontend/src/components/Link.tsx 7.69% <0.00%> (-79.81%) ⬇️
...et-frontend/src/SqlLab/components/TableElement.jsx 4.59% <0.00%> (-78.17%) ⬇️
... and 446 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62a15f0...3ec7d01. Read the comment docs.

@bkyryliuk
Copy link
Member Author

github checks somehow did not rerun when the commit was amended, still shows old code here:

image

@bkyryliuk
Copy link
Member Author

@dpgaspar, @villebro or @etr2460 could someone merge it, I am still waiting on getting the merge rights :(

@dpgaspar
Copy link
Member

dpgaspar commented May 8, 2020

merging, sorry for the delay

@dpgaspar dpgaspar merged commit 358bbe0 into apache:master May 8, 2020
khtruong pushed a commit to lyft/incubator-superset that referenced this pull request Jul 29, 2020
Co-authored-by: bogdan kyryliuk <bogdankyryliuk@dropbox.com>
khtruong added a commit to lyft/incubator-superset that referenced this pull request Jul 29, 2020
fix: Catch db_engine_spec.get_function_names exceptions (apache#9691)
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 0.37.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants