Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix docker.get_file for binary files #1076

Merged
merged 1 commit into from May 11, 2024

Conversation

matthijskooijman
Copy link
Contributor

The file to get is copied to a temp file, and then opened to read into memory. This would open the file in text mode (the default), which would fail on binary files that could not be decoded as utf8.

This changes the file opening to use binary mode. It also simplifies the rest of the code, which would try to handle both strings and bytes, but this is likely a remnant of py2 code (or the maybe copied from files.put where the passed file could possibly be a stream in text mode).

The file to get is copied to a temp file, and then opened to read into
memory. This would open the file in text mode (the default), which would
fail on binary files that could not be decoded as utf8.

This changes the file opening to use binary mode. It also simplifies the
rest of the code, which would try to handle both strings and bytes, but
this is likely a remnant of py2 code (or the maybe copied from files.put
where the passed file could possibly be a stream in text mode).
@matthijskooijman
Copy link
Contributor Author

To reproduce this error:

$ cat binary_test.py 
from pyinfra.operations import files
files.put(src="binary_file", dest="/binary_file")
files.get(src="/binary_file", dest="/tmp/binary_file")
$ echo -ne '\xf3' > binary_file
$ pyinfra @docker/debian:bookworm-slim binary_test.py --yes
--> Starting operation: files.get (src=/binary_file, dest=/tmp/binary_file)
Traceback (most recent call last):
  File "src/gevent/greenlet.py", line 908, in gevent._gevent_cgreenlet.Greenlet.run
  File "/home/matthijs/.local/pipx/venvs/pyinfra/lib/python3.11/site-packages/pyinfra/api/operations.py", line 184, in _run_host_op_with_context
    return run_host_op(state, host, op_hash)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/matthijs/.local/pipx/venvs/pyinfra/lib/python3.11/site-packages/pyinfra/api/operations.py", line 53, in run_host_op
    return _run_host_op(state, host, op_hash)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/matthijs/.local/pipx/venvs/pyinfra/lib/python3.11/site-packages/pyinfra/api/operations.py", line 121, in _run_host_op
    status = command.execute(state, host, connector_arguments)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/matthijs/.local/pipx/venvs/pyinfra/lib/python3.11/site-packages/pyinfra/api/command.py", line 191, in execute
    return host.get_file(
           ^^^^^^^^^^^^^^
  File "/home/matthijs/.local/pipx/venvs/pyinfra/lib/python3.11/site-packages/pyinfra/api/host.py", line 429, in get_file
    return self.connector.get_file(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/matthijs/.local/pipx/venvs/pyinfra/lib/python3.11/site-packages/pyinfra/connectors/docker.py", line 270, in get_file
    data = temp_f.read()
           ^^^^^^^^^^^^^
  File "<frozen codecs>", line 322, in decode
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xf3 in position 0: unexpected end of data
2024-03-21T21:54:39Z <Greenlet at 0x72ef51c844a0: _run_host_op_with_context(<pyinfra.api.state.State object at 0x72ef52013e50>, Host(@docker/debian:bookworm-slim), '9d1afe224119c48d16c1b791f0422627e757bed3')> failed with UnicodeDecodeError

I have not added any unit tests, since the normal docker unit tests patch out all of the actual docker stuff, so testing that the right calls are done does not seem meaningfull. Maybe a test could be added to the end-to-end tests, but it was not immediately clear to me if that would be a good idea and how to approach that (also, I need to get back to my actual project and pop a few levels of yak shaving here ;-p).

Copy link
Member

@Fizzadar Fizzadar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies this took so long to review @matthijskooijman, massive thank you for fixing this!

@Fizzadar Fizzadar merged commit 5643c01 into pyinfra-dev:3.x May 11, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants