-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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(dbview): Add token request button to DuckDB and MotherDuck database modal #27908
feat(dbview): Add token request button to DuckDB and MotherDuck database modal #27908
Conversation
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 is really nice, I love how easy it makes to connect to MotherDuck!
I left a few comments... can we also add some unit tests for build_sqlalchemy_uri
, get_parameters_from_uri
, and for the serialization/deserialization of DuckDBURI
?
@classmethod | ||
def parameters_json_schema(cls) -> Any: | ||
""" | ||
Return configuration parameters as OpenAPI. | ||
""" | ||
if not cls.parameters_schema: | ||
return None | ||
|
||
spec = APISpec( | ||
title="Database Parameters", | ||
version="1.0.0", | ||
openapi_version="3.0.2", | ||
plugins=[MarshmallowPlugin()], | ||
) | ||
spec.components.schema(cls.__name__, schema=cls.parameters_schema) | ||
return spec.to_dict()["components"]["schemas"][cls.__name__] |
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.
Note to self, we should not require this, it's the same for every database.
@guenp @betodealmeida What's our policy for this screen regarding specific database configs? I thought this was a generic database interface given that it would be really difficult to maintain this modal if we add UI-specific elements for each database type which seems the case here. Are we planning to support specific versions of this modal for each database type? |
@michael-s-molina the frontend components are mostly generic, and the configuration lives in the DB engine spec (eg, https://github.com/apache/superset/pull/27908/files#diff-651962bab25f2e142c7d1701b15bed4e3c055374d855f1340b85724b573966b7R116-R129). In this PR they're adding some optional parameters to show the URL to get the token, which is useful in general, IMHO. That being said, I actually think that the frontend components should be more specific. I think we should have a |
That was exactly my thinking. It's not a blocker for this PR but something we should consider given the flexibility we'll gain. It's similar to database engine specs where you have common elements but can also extend them to include specific things that will improve overall UX for users. |
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
24b4c93
to
9eebe83
Compare
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.
Awesome!
…ase modal (apache#27908) Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
…ase modal (apache#27908) Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
…ase modal (apache#27908) Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
…ase modal (apache#27908) Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
SUMMARY
DuckDBParametersSchema
to break out the DuckDB connection URI into database, access token (for MotherDuck) and queryThe latter is implemented by adding support for the
metadata
description
andload_default
values from the parameters schema fields.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
See animated GIF.
ADDITIONAL INFORMATION