[GitHub] [airflow] potiuk commented on a change in pull request #17273: Switch to 'smbprotocol' library

[GitBox <gi...@apache.org>]

potiuk commented on a change in pull request #17273:
URL: https://github.com/apache/airflow/pull/17273#discussion_r678357257



##########
File path: airflow/providers/samba/hooks/samba.py
##########
@@ -16,67 +16,229 @@
 # specific language governing permissions and limitations
 # under the License.
 
-import os
+import posixpath
+from functools import wraps
+from shutil import copyfileobj
+from typing import Optional
 
-from smbclient import SambaClient
+import smbclient
 
 from airflow.hooks.base import BaseHook
 
 
 class SambaHook(BaseHook):
-    """Allows for interaction with an samba server."""
+    """Allows for interaction with a Samba server.
+
+    :param samba_conn_id: The connection id reference.
+    :type samba_conn_id: str
+    :param share:
+        An optional share name. If this is unset then the "schema" field of
+        the connection is used in its place.
+    :type share: str
+    """
 
     conn_name_attr = 'samba_conn_id'
     default_conn_name = 'samba_default'
     conn_type = 'samba'
     hook_name = 'Samba'
 
-    def __init__(self, samba_conn_id: str = default_conn_name) -> None:
+    def __init__(self, samba_conn_id: str = default_conn_name, share: Optional[str] = None) -> None:
         super().__init__()
-        self.conn = self.get_connection(samba_conn_id)
+        conn = self.get_connection(samba_conn_id)
+
+        if not conn.login:
+            self.log.info("Login not provided")
+
+        if not conn.password:
+            self.log.info("Password not provided")
+
+        self._host = conn.host
+        self._share = share or conn.schema
+        self._connection_cache = connection_cache = {}
+        self._conn_kwargs = {
+            "username": conn.login,
+            "password": conn.password,
+            "port": conn.port or 445,
+            "connection_cache": connection_cache,
+        }
+
+    def __enter__(self):
+        # This immediately connects to the host (which can be

Review comment:
       If I read that correctly - it means that SambaHook MUST be used as ContextManager in some cases, otherwise there might some problems with initializing some parameters during constructor? This is not a usual pattern we have in Airflow for hooks (though I think it's nice pattern for Hooks) but I think some explanation is needed at least in the docstring explaining the difference between the two and when to use it? 
   
   Also - would you mind to add a CHANGELOG.txt entry? Don't yet put the version (I will update it) but some backwards-compatibility notes are needed (how to migrate?)




-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org