Re: [PR] feat(db): Adding DB_ENGINE_URI_VALIDATOR [superset]
dpgaspar commented on code in PR #27847:
URL: https://github.com/apache/superset/pull/27847#discussion_r1547514948
##
superset/config.py:
##
@@ -1206,6 +1206,17 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC( # pylint:
disable=invalid-name
DB_CONNECTION_MUTATOR = None
+# A callable that is invoked for every invocation of DB Engine Specs
+# which allows for custom validation of the engine URI.
+# See: superset.db_engine_specs.base.BaseEngineSpec.validate_database_uri
+# Example:
+# def DB_ENGINE_URI_VALIDATOR(sqlalchemy_uri: URL):
+# if not :
+# raise Exception("URI invalid")
+#
+DB_ENGINE_URI_VALIDATOR = None
Review Comment:
nit: `DB_ENGINE_URI_VALIDATOR: Callable[[URL], None] | None = None`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(db): Adding DB_ENGINE_URI_VALIDATOR [superset]
betodealmeida commented on PR #27847: URL: https://github.com/apache/superset/pull/27847#issuecomment-2030995464 Yeah, in general there are two parts: engine = create_engine(uri, **params) Both are needed for a full connection, since in some cases the params are too big to build a URI (like BigQuery credentials). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(db): Adding DB_ENGINE_URI_VALIDATOR [superset]
mistercrunch commented on PR #27847: URL: https://github.com/apache/superset/pull/27847#issuecomment-2030992466 I think the bigger exception is the way we do BigQuery for instance cramming many things into a blob, but unclear how common/uncommon it is. This probably a good starting point. Can add more hooks if/as needed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(db): Adding DB_ENGINE_URI_VALIDATOR [superset]
craig-rueda commented on PR #27847: URL: https://github.com/apache/superset/pull/27847#issuecomment-2030982198 Aren't all of the connection attributes captured in the parsed url? Also, this is intended to be invoked pre-connect, so we likely wouldn't want to perform validations on a "connection". -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(db): Adding DB_ENGINE_URI_VALIDATOR [superset]
codecov[bot] commented on PR #27847: URL: https://github.com/apache/superset/pull/27847#issuecomment-2030786537 ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/27847?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report Attention: Patch coverage is `66.7%` with `1 lines` in your changes are missing coverage. Please review. > Project coverage is 67.20%. Comparing base [(`ca47717`)](https://app.codecov.io/gh/apache/superset/commit/ca47717eb0c35c1e56fa2fc4f0dee16dec203bf0?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`f0ac4e1`)](https://app.codecov.io/gh/apache/superset/pull/27847?dropdown=coverage=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). | [Files](https://app.codecov.io/gh/apache/superset/pull/27847?dropdown=coverage=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Patch % | Lines | |---|---|---| | [superset/db\_engine\_specs/base.py](https://app.codecov.io/gh/apache/superset/pull/27847?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/27847?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Additional details and impacted files ```diff @@Coverage Diff @@ ## master #27847 +/- ## == - Coverage 67.41% 67.20% -0.21% == Files1920 1920 Lines 7524275218 -24 Branches 8423 8423 == - Hits5072450553 -171 - Misses 2245722604 +147 Partials 2061 2061 ``` | [Flag](https://app.codecov.io/gh/apache/superset/pull/27847/flags?src=pr=flags_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [hive](https://app.codecov.io/gh/apache/superset/pull/27847/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `48.93% <66.66%> (?)` | | | [mysql](https://app.codecov.io/gh/apache/superset/pull/27847/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `?` | | | [postgres](https://app.codecov.io/gh/apache/superset/pull/27847/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `?` | | | [presto](https://app.codecov.io/gh/apache/superset/pull/27847/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `53.63% <66.66%> (?)` | | | [python](https://app.codecov.io/gh/apache/superset/pull/27847/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `77.71% <66.66%> (-0.42%)` | :arrow_down: | | [sqlite](https://app.codecov.io/gh/apache/superset/pull/27847/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `77.43% <66.66%> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/27847?dropdown=coverage=pr=continue_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
