Add support for extra parameters to samba client #16115
Conversation
| samba = SambaClient( | ||
| server=self.conn.host, | ||
| share=self.conn.schema, | ||
| username=self.conn.login, | ||
| ip=self.conn.host, | ||
| password=self.conn.password, | ||
| **self.conn.extra_dejson, |
There was a problem hiding this comment.
Is the list of fields exhaustive? I feel we should do some sanitising instead of passing the whole JSON object directly in.
Also :param is for function arguments and should not be used for extra keys. These descriptions should live somewhere else.
There was a problem hiding this comment.
@uranusjr fixed in 0c8cca7 – the list is inexhaustive (mentioned now) and I have reformatted as definition list.
The :param stuff was token from the Oracle hook, but I guess that is a bad example :-)
By the way, I have since realized that the underlying Python library that supports SambaHook is basically abandonware and not particularly useful – it breaks quite a few expectations if you look at the actual smbclient functionality.
There was a problem hiding this comment.
By the way, I have since realized that the underlying Python library that supports
SambaHookis basically abandonware and not particularly useful
Thanks for taising this. It would be worthwhile to open an issue for this to migrate to another library, I think. I don’t have much experience dealing with SMB with Python, but pysmb seems to be a popular choice.
There was a problem hiding this comment.
There is an open issue: #14054
There was a problem hiding this comment.
@uranusjr so I think we can perhaps go ahead and consider merging this request and then I'll submit another request to migrate to pysmb.
I have already done some testing and it seems very well suited.
There was a problem hiding this comment.
Yup LGTM. Could you close-reopen the PR to trigger CI?
There was a problem hiding this comment.
@malthe
Can you please rebase to latest main (former master branch)?
There was a problem hiding this comment.
@eladkal looks about ready now, checks have run.
malthe
force-pushed
the
add-extra-json-to-samba-hook
branch
from
0c8cca7
to
b41b0be
Compare
|
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
github-actions
bot
added
the
okay to merge
|
@malthe can you fix the static test? |
|
@eladkal fixed – I don't really understand this automated build, but the static test seems to be skipped now. |
malthe
force-pushed
the
add-extra-json-to-samba-hook
branch
from
0f6217a
to
b1b0297
Compare
| @@ -128,7 +128,7 @@ def test_wait_for_job_cancelled(self, mock_get_job): | |||
|
|
|||
| @requests_mock.mock() | |||
| def test_connection_success(self, m): | |||
| m.get(self.health_endpoint, status_code=200,) | |||
| m.get(self.health_endpoint, status_code=200) | |||
There was a problem hiding this comment.
This change is not related :)
We also had a broken CI so another rebase so fix the failing tests
malthe
force-pushed
the
add-extra-json-to-samba-hook
branch
from
eb193b8
to
88c2079
Compare
|
@eladkal should be ready now. |
malthe
force-pushed
the
add-extra-json-to-samba-hook
branch
from
88c2079
to
ea4e6aa
Compare
This adds the option to use the extra field for adding additional parameters to the
SambaClientconstructor such asworkgroupusing JSON-formatting.Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.