Taragolis commented on code in PR #38466:
URL: https://github.com/apache/airflow/pull/38466#discussion_r1543749181
##########
airflow/providers/docker/operators/docker.py:
##########
@@ -283,6 +283,8 @@ def __init__(
self.dns = dns
self.dns_search = dns_search
self.docker_url = docker_url or os.environ.get("DOCKER_HOST") or
"unix://var/run/docker.sock"
+ if not isinstance(self.docker_url, list):
+ self.docker_url = [self.docker_url]
Review Comment:
```suggestion
```
Not required since it handled in hook
##########
airflow/providers/docker/hooks/docker.py:
##########
@@ -146,15 +151,23 @@ def construct_tls_config(
@cached_property
def api_client(self) -> APIClient:
"""Create connection to docker host and return ``docker.APIClient``
(cached)."""
- client = APIClient(
- base_url=self.__base_url, version=self.__version, tls=self.__tls,
timeout=self.__timeout
- )
- if self.docker_conn_id:
- # Obtain connection and try to login to Container Registry only if
``docker_conn_id`` set.
- self.__login(client, self.get_connection(self.docker_conn_id))
-
- self._client_created = True
- return client
+ for url in self.__base_url:
+ try:
+ client = APIClient(
+ base_url=url, version=self.__version, tls=self.__tls,
timeout=self.__timeout
+ )
+ if not client.ping():
+ raise ConnectionError("Failed to ping host %s.", url)
+ if self.docker_conn_id:
+ # Obtain connection and try to login to Container Registry
only if ``docker_conn_id`` set.
+ self.__login(client,
self.get_connection(self.docker_conn_id))
+ self._client_created = True
+ return client
Review Comment:
Move this in else block
```python
try:
...
except:
...
else:
self._client_created = True
return client
```
##########
airflow/providers/docker/hooks/docker.py:
##########
@@ -62,20 +62,25 @@ class DockerHook(BaseHook):
def __init__(
self,
docker_conn_id: str | None = default_conn_name,
- base_url: str | None = None,
+ base_url: str | list[str] | None = None,
version: str | None = None,
tls: TLSConfig | bool | None = None,
timeout: int = DEFAULT_TIMEOUT_SECONDS,
) -> None:
super().__init__()
if not base_url:
raise AirflowException("URL to the Docker server not provided.")
- elif tls:
- if base_url.startswith("tcp://"):
- base_url = base_url.replace("tcp://", "https://";)
- self.log.debug("Change `base_url` schema from 'tcp://' to
'https://'.")
- if not base_url.startswith("https://";):
- self.log.warning("When `tls` specified then `base_url`
expected 'https://' schema.")
+ if not isinstance(base_url, list):
Review Comment:
```suggestion
if isinstance(base_url, str):
```
##########
airflow/providers/docker/hooks/docker.py:
##########
@@ -146,15 +151,23 @@ def construct_tls_config(
@cached_property
def api_client(self) -> APIClient:
"""Create connection to docker host and return ``docker.APIClient``
(cached)."""
- client = APIClient(
- base_url=self.__base_url, version=self.__version, tls=self.__tls,
timeout=self.__timeout
- )
- if self.docker_conn_id:
- # Obtain connection and try to login to Container Registry only if
``docker_conn_id`` set.
- self.__login(client, self.get_connection(self.docker_conn_id))
-
- self._client_created = True
- return client
+ for url in self.__base_url:
+ try:
+ client = APIClient(
+ base_url=url, version=self.__version, tls=self.__tls,
timeout=self.__timeout
+ )
+ if not client.ping():
+ raise ConnectionError("Failed to ping host %s.", url)
Review Comment:
```suggestion
msg = f"Failed to ping host {url}."
raise ConnectionError(msg)
```
##########
airflow/providers/docker/operators/docker.py:
##########
@@ -283,6 +283,8 @@ def __init__(
self.dns = dns
self.dns_search = dns_search
self.docker_url = docker_url or os.environ.get("DOCKER_HOST") or
"unix://var/run/docker.sock"
+ if not isinstance(self.docker_url, list):
+ self.docker_url = [self.docker_url]
Review Comment:
Better move convert into the hook, ho knows maybe one day `docker_url`
become a templated field and hook called after the templates rendered
##########
airflow/providers/docker/hooks/docker.py:
##########
@@ -62,20 +62,25 @@ class DockerHook(BaseHook):
def __init__(
self,
docker_conn_id: str | None = default_conn_name,
- base_url: str | None = None,
+ base_url: str | list[str] | None = None,
version: str | None = None,
tls: TLSConfig | bool | None = None,
timeout: int = DEFAULT_TIMEOUT_SECONDS,
) -> None:
super().__init__()
if not base_url:
raise AirflowException("URL to the Docker server not provided.")
- elif tls:
- if base_url.startswith("tcp://"):
- base_url = base_url.replace("tcp://", "https://";)
- self.log.debug("Change `base_url` schema from 'tcp://' to
'https://'.")
- if not base_url.startswith("https://";):
- self.log.warning("When `tls` specified then `base_url`
expected 'https://' schema.")
+ if not isinstance(base_url, list):
+ base_url = [base_url]
+ if tls:
+ for url in base_url:
+ if url.startswith("tcp://"):
+ self.log.debug("Change `base_url` schema from 'tcp://' to
'https://'.")
+ elif not url.startswith("https://";):
+ self.log.warning("When `tls` specified then `base_url`
expected 'https://' schema.")
Review Comment:
Maybe move it into the private method and simply call:
```python
def _redact_tls_schema(self, url: str) -> str:
if base_url.startswith("tcp://"):
base_url = base_url.replace("tcp://", "https://";)
self.log.debug("Change `base_url` schema from 'tcp://' to
'https://'.")
if not base_url.startswith("https://";):
self.log.warning("When `tls` specified then `base_url` expected
'https://' schema.")
return base_url
base_url = list(map(self._redact_tls_schema, base_url))
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
