Repository: canonical/operator

Issue:
`Container.push_path()` only supports text files
As you can see in the linked code below, the `Container.push_path()` function only works with text files. Internally, it's calling `Container.push()`, but it doesn't allow customizing its parameters, so you can't specify *encoding*, *permissions*, *user_id* and so on.

https://github.com/canonical/operator/blob/77b85ee2ce8e9319f89aef443c7337684f4fd21d/ops/model.py#L2594-L2598

It would be great if at least the `encoding` parameter was surfaced in `push_path`, so that it's possible to get binary files from the charm container.

My current workaround:
```python
with open(binary_file, "rb") as f:
	container.push(dst_path, source=f)
```

Discussion:
benhoyt: > Previously, line endings would get normalised to \n, since Python does that when opening in text mode.

Yeah, I'd consider that a subtle but nasty bug. I was involved in the original design of push and pull, and the intention with push and pull was very much (and especially the recursive push_path and pull_path versions) was copying files, not modifying the data or doing line ending normalisation. In Go it's all bytes anyway. In Python, push and pull themselves allow supply strings which are then encoded as UTF-8, but that's just to make passing strings/TextIO easier in Python, not for normalisation reasons.

I think one test is probably fine here as the intent is that there's only code path, so going to run with this. Thanks!

Patch:
diff --git a/ops/model.py b/ops/model.py
index a87001e..a86c2db 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -2662,7 +2662,7 @@ class Container:
                     if info.type is pebble.FileType.DIRECTORY:
                         self.make_dir(dstpath, make_parents=True)
                         continue
-                    with open(info.path) as src:
+                    with open(info.path, 'rb') as src:
                         self.push(
                             dstpath,
                             src,


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index 01df174..541eeb1 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -1369,8 +1369,8 @@ def test_recursive_push_and_pull(case: PushPullCase):
     for file in case.files:
         fpath = os.path.join(push_src.name, file[1:])
         os.makedirs(os.path.dirname(fpath), exist_ok=True)
-        with open(fpath, 'w') as f:
-            f.write('hello')
+        with open(fpath, 'wb') as f:
+            f.write(b'push \xc3\x28')  # invalid UTF-8 to ensure binary works
     if case.dirs:
         for directory in case.dirs:
             fpath = os.path.join(push_src.name, directory[1:])
@@ -1402,6 +1402,8 @@ def test_recursive_push_and_pull(case: PushPullCase):
     ), f'push_path gave wrong expected errors: want {case.errors}, got {errors}'
     for fpath in case.want:
         assert c.exists(fpath), f'push_path failed: file {fpath} missing at destination'
+        content = c.pull(fpath, encoding=None).read()
+        assert content == b'push \xc3\x28'
     for fdir in case.want_dirs:
         assert c.isdir(fdir), f'push_path failed: dir {fdir} missing at destination'