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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org