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 so why should it remain broken? 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 will do it correctly only if you are experienced with Airflow JSON format
and its quirks. After my proposal you could write it:
```
"example_https": {
"conn_type": "https",
"host": "example.com"
"schema": "endpoint"
}
```
(or in the URI format https://example.com/endpoint).
> 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
