Repository: datalad/datalad-next

Issue:
push to export remote patch swallows errors
This `try ... except`: https://github.com/datalad/datalad-next/blob/864896e26af7361b739b05df4fcd302b0b407db3/datalad_next/patches/push_to_export_remote.py#L216

is wrong. If `annex export` fails we get an error result from `_call_annex_records_items_` rather than an exception. And this error result is unconditionally turned into an `status='ok'` record here.

Discussion:

Patch:
diff --git a/changelog.d/20220812_190404_benjaminpoldrack_fix_push_patch.md b/changelog.d/20220812_190404_benjaminpoldrack_fix_push_patch.md
new file mode 100644
index 0000000..b092504
--- /dev/null
+++ b/changelog.d/20220812_190404_benjaminpoldrack_fix_push_patch.md
@@ -0,0 +1,7 @@
+### 🐛 Bug Fixes
+
+- Fixed datalad-push always reporting success when pushing to
+  an export remote.
+  Fixes https://github.com/datalad/datalad-next/issues/88 via
+  https://github.com/datalad/datalad-next/pull/93 (by @bpoldrack)
+
diff --git a/datalad_next/patches/push_to_export_remote.py b/datalad_next/patches/push_to_export_remote.py
index d6b82ed..432f209 100644
--- a/datalad_next/patches/push_to_export_remote.py
+++ b/datalad_next/patches/push_to_export_remote.py
@@ -167,6 +167,8 @@ def _transfer_data(repo: AnnexRepo,
         )
         return
 
+    from datalad.interface.results import annexjson2result
+
     # TODO:
     #  - check for configuration entries, e.g. what to export
 
@@ -221,12 +223,13 @@ def _transfer_data(repo: AnnexRepo,
                 ],
                 progress=True
             ):
-                yield {
-                    **res_kwargs,
-                    "action": "copy",
-                    "status": "ok",
-                    "path": str(Path(res_kwargs["path"]) / result["file"])
-                }
+                result_adjusted = \
+                    annexjson2result(result, ds, **res_kwargs)
+                # annexjson2result overwrites 'action' with annex' 'command',
+                # even if we provided our 'action' within res_kwargs. Therefore,
+                # change afterwards instead:
+                result_adjusted['action'] = "copy"
+                yield result_adjusted
 
         except CommandError as cmd_error:
             ce = CapturedException(cmd_error)


Test patch:
diff --git a/datalad_next/patches/tests/test_push_to_export_remote.py b/datalad_next/patches/tests/test_push_to_export_remote.py
index 3b0ef55..bb51c0d 100644
--- a/datalad_next/patches/tests/test_push_to_export_remote.py
+++ b/datalad_next/patches/tests/test_push_to_export_remote.py
@@ -11,6 +11,7 @@ from datalad.tests.utils_pytest import (
     SkipTest,
     assert_false,
     assert_in,
+    assert_in_results,
     assert_true,
     eq_,
 )
@@ -55,11 +56,19 @@ class MockRepo:
 
     def _call_annex_records_items_(self, *args, **kwargs):
         yield {
-            'target': args[0][3],
-            'action': 'copy',
-            'status': 'ok',
+            "command": f"export {args[0][3]}",
             "file": "file.txt",
+            "success": True,
+            "input": [],
+            "error-messages": []
         }
+        yield {
+            "command": f"export {args[0][3]}",
+            "success": False,
+            "input": [],
+            "error-messages":
+                ["external special remote error: WHATEVER WENT WRONG"],
+            "file": "somefile"}
 
 
 def _call_transfer(target: str,
@@ -67,6 +76,7 @@ def _call_transfer(target: str,
                    return_special_remotes: bool = True) -> Generator:
     ds_mock = MagicMock()
     ds_mock.config.getbool.return_value = config_result
+    ds_mock.pathobj = Path("/root")
     return _transfer_data(
         repo=MockRepo(return_special_remotes),
         ds=ds_mock,
@@ -107,14 +117,16 @@ def test_patch_execute_export():
         gele_mock.return_value = None
         results = tuple(_call_transfer("yes-target", False))
         eq_(pd_mock.call_count, 0)
-        assert_in(
-            {
-                "path": str(Path("/root/file.txt")),
-                "target": "yes-target",
-                "action": "copy",
-                "status": "ok",
-            },
-            results)
+        assert_in_results(results,
+                          path=str(Path("/root/file.txt")),
+                          target="yes-target",
+                          action="copy",
+                          status="ok")
+        assert_in_results(results,
+                          path=str(Path("/root/somefile")),
+                          target="yes-target",
+                          action="copy",
+                          status="error")
 
 
 def test_patch_skip_ignore_targets_export():
@@ -144,14 +156,16 @@ def test_patch_check_envpatch():
         gc_mock.return_value = {"secret": "abc", "user": "hans"}
         results = tuple(_call_transfer("yes-target", False))
         eq_(pd_mock.call_count, 0)
-        assert_in(
-            {
-                "path": str(Path("/root/file.txt")),
-                "target": "yes-target",
-                "action": "copy",
-                "status": "ok",
-            },
-            results)
+        assert_in_results(results,
+                          path=str(Path("/root/file.txt")),
+                          target="yes-target",
+                          action="copy",
+                          status="ok")
+        assert_in_results(results,
+                          path=str(Path("/root/somefile")),
+                          target="yes-target",
+                          action="copy",
+                          status="error")
 
 
 def test_no_special_remotes():