Skip to content

Open src and dst in binary for samba copy#18752

Merged
uranusjr merged 1 commit into
apache:mainfrom
Opus-Interactive:fixSambaPushFromLocalOpenBinary
Oct 7, 2021
Merged

Open src and dst in binary for samba copy#18752
uranusjr merged 1 commit into
apache:mainfrom
Opus-Interactive:fixSambaPushFromLocalOpenBinary

Conversation

@fredthomsen

@fredthomsen fredthomsen commented Oct 5, 2021

Copy link
Copy Markdown
Contributor

Be consistent with how files are opened when using samba to push local
files to remote. Opening files in binay in this case is safer since we
don't care about their contents.

Without this fix the following traceback occurs:

Traceback (most recent call last):
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/models/taskinstance.py", line 1165, in _run_raw_task
    self._prepare_and_execute_task_with_callbacks(context, task)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/models/taskinstance.py", line 1283, in _prepare_and_execute_task_with_callbacks
    result = self._execute_task(context, task_copy)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/models/taskinstance.py", line 1313, in _execute_task
    result = task_copy.execute(context=context)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/operators/python.py", line 150, in execute
    return_value = self.execute_callable()
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/operators/python.py", line 161, in execute_callable
    return self.python_callable(*self.op_args, **self.op_kwargs)
  File "/opt/airflow/dags/vmware_license_billing.py", line 389, in reports_to_fs01
    SambaHook().push_from_local(
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/providers/samba/hooks/samba.py", line 247, in push_from_local
    copyfileobj(f, g)
  File "/usr/local/lib/python3.8/shutil.py", line 208, in copyfileobj
    fdst_write(buf)
TypeError: write() argument must be str, not bytes

^ Add meaningful description above

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.

Be consistent with how files are opened when using samba to push local
files to remote.  Opening files in binay in this case is safer since we
don't care about their contents.
@github-actions github-actions Bot added the okay to merge It's ok to merge this PR as it does not require more tests label Oct 5, 2021
@github-actions

github-actions Bot commented Oct 5, 2021

Copy link
Copy Markdown
Contributor

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.

@uranusjr

uranusjr commented Oct 5, 2021

Copy link
Copy Markdown
Member

Would it be possible to add a test for this?

@fredthomsen

Copy link
Copy Markdown
Contributor Author

Would it be possible to add a test for this?

I struggled to think of a test that can add any value here while mocking all interactions with the underlying smbprotocol library. Mocking open and the open_file method to ensure they are both opened using binary and/or ensuring that copyfileobj only takes file-like objects whose read and write methods expect binary strings seems like the only options, but I didn't that having that as a test helped.

@potiuk

potiuk commented Oct 6, 2021

Copy link
Copy Markdown
Member

Would it be possible to add a test for this?

I struggled to think of a test that can add any value here while mocking all interactions with the underlying smbprotocol library. Mocking open and the open_file method to ensure they are both opened using binary and/or ensuring that copyfileobj only takes file-like objects whose read and write methods expect binary strings seems like the only options, but I didn't that having that as a test helped.

My thoughts exactly. For me that's one of those fixes for which the unit tests bring no value, just add complexitty really.

@potiuk potiuk closed this Oct 6, 2021
@potiuk potiuk reopened this Oct 6, 2021
@potiuk

potiuk commented Oct 6, 2021

Copy link
Copy Markdown
Member

REopened to rebuild

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants