add https support for Druid connector#27634
add https support for Druid connector#27634Nitin2332 wants to merge 1 commit intoopen-metadata:mainfrom
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
| if connection.scheme.value == "druid+https": | ||
| if not connection.connectionArguments: | ||
| connection.connectionArguments = init_empty_connection_arguments() | ||
| connection.connectionArguments.root["use_https"] = True |
There was a problem hiding this comment.
💡 Quality: New HTTPS connection logic in get_connection() is untested
The new test test_druid_https_url only verifies URL generation via get_connection_url(), but the actual new logic that injects use_https=True into connectionArguments (lines 48-51 of connection.py) lives in get_connection() and is not covered by any test. If someone later refactors this code and breaks the use_https injection, no test would catch it.
Suggested fix:
Add a test that calls `get_connection` (or directly exercises the `get_connection` logic) with `scheme=druid+https` and asserts that `connection.connectionArguments.root["use_https"]` is `True` after the function runs. You could also verify the case where `connectionArguments` is already set to ensure it's preserved rather than overwritten.
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsAdds HTTPS support for the Druid connector by updating connection arguments. Please add unit tests to verify the new HTTPS connection logic in 💡 Quality: New HTTPS connection logic in get_connection() is untested📄 ingestion/src/metadata/ingestion/source/database/druid/connection.py:48-51 📄 ingestion/tests/unit/test_source_connection.py:656-666 The new test Suggested fix🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
Describe your changes:
Fixes #25991
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
druid+httpsoption todruidSchemeindruidConnection.json.connection.pyto setuse_httpsin connection arguments whendruid+httpsscheme is selected.test_source_connection.pyto verify HTTPS connection URL generation.Druid.mdto provide instructions on usingdruid+httpsfor HTTPS connections.This will update automatically on new commits.