Switch to 'smbprotocol' library #17273
Conversation
| } | ||
|
|
||
| def __enter__(self): | ||
| # This immediately connects to the host (which can be |
There was a problem hiding this 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?)
There was a problem hiding this comment.
@potiuk where would those notes go? I don't see other changelog entries with such information. I have added a simple changelog entry for now and a note in the docstring about using it as a context manager.
There was a problem hiding this comment.
You could see some of those in other providers - for example look at the Google provider: https://github.com/apache/airflow/blob/main/airflow/providers/google/CHANGELOG.rst - usually when there are braking changes that require more than one-line mentioning.
Even in Samba, we've added a warning when we removed "apply_default" (this was a global one and it actually did NOT really apply to samba but it went there as it was "all operator" change.
https://github.com/apache/airflow/blob/main/airflow/providers/samba/CHANGELOG.rst
github-actions
bot
added
the
full tests needed
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
malthe
force-pushed
the
issue-14054-smbprotocol
branch
3 times, most recently
from
b7a927c
to
8f75fbe
Compare
CHANGELOG.txt
Outdated
| Improvements | ||
| """""""""""" | ||
|
|
||
| - The Samba hook now uses the `smbprotocol` library, exposing a much | ||
| bigger set of functionality and supporting SMB2/3 protocols. |
There was a problem hiding this comment.
This is in wrong file -- it should be at https://github.com/apache/airflow/blob/main/airflow/providers/samba/CHANGELOG.rst
There was a problem hiding this comment.
@kaxil thanks – I have removed this entry again. I had actually already written a more detailed changelog entry in the provider changelog.
malthe
force-pushed
the
issue-14054-smbprotocol
branch
from
c899e3f
to
7608301
Compare
This avoids contaminating the global namespace which could lead to subtle errors.
See https://bugs.python.org/issue21269 for details.
malthe
force-pushed
the
issue-14054-smbprotocol
branch
from
7608301
to
45933ae
Compare
|
@uranusjr this seems to fail for no apparant reason. |
|
seems like the errors were rather unrelated "stability" problems. Merging. |
This change switches the Samba hook implementation to the smbprotocol library.
As discussed in #14054 the current implementation uses an unmaintained and outdated library.
In addition, the implementation suggested here conveniently exposes a large part of the user-friendly API provided by the underlying library. For example, to use the
smbclient.getxattrfunction, one would callsamba_hook.getxattr(with connection details being automatically provided).closes: #14054