-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
feat: Database Connection UI #14881
feat: Database Connection UI #14881
Conversation
…ax/db-connection-ui
Codecov Report
@@ Coverage Diff @@
## master #14881 +/- ##
==========================================
- Coverage 77.16% 76.82% -0.34%
==========================================
Files 975 976 +1
Lines 50865 51217 +352
Branches 6740 6893 +153
==========================================
+ Hits 39251 39349 +98
- Misses 11398 11649 +251
- Partials 216 219 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* poc picker for db selection * working select * setup is loading for available dbs and step1 view * fix on close * update on fetch * remove unneeded code * add some styls
2d2dd39
to
ef63e3c
Compare
…ax/db-connection-ui
…uperset into pexdax/db-connection-ui
id="selectedFile" | ||
className="input-upload" | ||
type="file" | ||
onChange={async event => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit, but this is a fairly long function in this event handler. Not necessarily required now, but I think it would read better if we pulled this out into a named function instead.
* chore: simplify errors and issue codes (#15437) * Fix issue number * Fix test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I left a few nits and TODOs that we can tackle after merging.
PREFERRED_DATABASES: List[str] = [ | ||
# "PostgreSQL", | ||
# "Presto", | ||
# "MySQL", | ||
# "SQLite", | ||
"PostgreSQL", | ||
"Presto", | ||
"MySQL", | ||
"SQLite", | ||
# etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we wanna curate this list better? I just wrote these of the top of my head.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are fine for now
@yousoph what do you think?
@@ -35,6 +35,8 @@ | |||
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType | |||
from superset.models.core import Database | |||
|
|||
BYPASS_VALIDATION_ENGINES = ["bigquery"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: make this a set
superset/databases/schemas.py
Outdated
serialized_encrypted_extra = ( | ||
data.get("encrypted_extra", "{}") | ||
if data.get("encrypted_extra") | ||
else "{}" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
serialized_encrypted_extra = ( | |
data.get("encrypted_extra", "{}") | |
if data.get("encrypted_extra") | |
else "{}" | |
) | |
serialized_encrypted_extra = data.get("encrypted_extra") or "{}" |
Your database was successfully connected! Here are some optional | ||
settings for your database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the strings in this file need to be translatable, but we can do that later.
@@ -59,20 +60,20 @@ const ExtraOptions = ({ | |||
<div> | |||
<h4>SQL Lab</h4> | |||
<p className="helper"> | |||
Configure how this database will function in SQL Lab. | |||
Adjust how this database will interact with SQL Lab. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more here that need to be translatable.
…uperset into pexdax/db-connection-ui
* fix: add icons (#15122) * added alerts * revisions * added icon * spinner * added to error map
* fix: add icons (#15122) * added alerts * revisions * added icon * spinner * this will test * fixed test
* send parameters if they are available * fix bigquery sqlalchem issue * fix casting
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
Fully revamped the DB Connection Flow for users to easily add their databases to the metastore. With the new DB Connection Modal, we are able to build custom forms (dynamic forms) for whatever databases/engines we want. If their isn't a dynamic form available all dbs default to sqlalchemy for connecting.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2021-06-22.at.5.24.03.PM.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION