Bisk1 commented on code in PR #35712: URL: https://github.com/apache/airflow/pull/35712#discussion_r1398008829
########## airflow/providers/apache/livy/hooks/livy.py: ########## @@ -575,10 +575,11 @@ def _generate_base_url(self, conn: Connection) -> str: if conn.host and "://" in conn.host: base_url: str = conn.host else: - # schema defaults to HTTP - schema = conn.schema if conn.schema else "http" + scheme = conn.conn_type Review Comment: > Some of the connections also use additional fields Http-based hooks don't use additional fields, some of them hide or re-label fields but it's cosmetic - no functionality is missing without this. > URI not the only one way to define connection, and to be honest not he easiest way to create a connection This is subjective... JSON format is newer but both have its uses. People who specify connection with env variables will want a one-liner. And URI format is still supported by Airflow. Also, JSON format has problematic names. Suppose you want to set up connection to `https://example.com/endpoint`, then configuration would be like this: ``` "example_https": { "conn_type": "http", "host": "https://example.com/endpoint" } ``` but clearly `https://example.com/endpoint` is not a 'host', it's a URL so you need to be experienced with JSON format to do it correctly. After the change you could write it: ``` "example_https": { "conn_type": "https", "host": "example.com" "schema": "endpoint" } ``` (or in the URI format). > This action is required to create additional Hook for handle this "feature". Can you elaborate what do you mean? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org