raising a NonzeroReturnCode when download fails (#81)

* raising a `Nonzero return code` when download fails

(also cleaning up)

* doing the same for exports and FileAnnotations

* adding `--ignore_errors` option

* making the flake8 gods happy

* updating ome-types and cli-transfer versions

* added tests, updated requirements

* added `--ignore_errors` to readme

.github/workflows/omero_plugin.yml

@@ -36,5 +36,7 @@ jobs:
           repository: ome/omero-test-infra
           path: .omero
           ref: ${{ secrets.OMERO_TEST_INFRA_REF }}
-      - name: Build and run OMERO tests
+      - name: Build and run standard tests
         run: POLICY_BINARY_ACCESS=+read,+write,+image,+plate .omero/docker $STAGE
+      - name: Build and run no-plate tests
+        run: .omero/docker $STAGE

.omeroci/cli-build

@@ -8,6 +8,8 @@ set -x
 TARGET=${TARGET:-..}
 cd $TARGET
 
+BIN_ACCESS=$(/opt/omero/server/venv3/bin/omero config get omero.policy.binary_access)
+
 GUESS=${PWD#*omero-cli-*}
 PLUGIN=${PLUGIN:-$GUESS}
 
@@ -22,5 +24,8 @@ conda activate omero
 
 export OMERO_DIST=${OMERO_DIST:-/opt/omero/server/OMERO.server}
 omero $PLUGIN -h
-
-python setup.py test -t test -i ${OMERO_DIST}/etc/ice.config -vs
+if [[ $BIN_ACCESS =~ "+plate" ]]; then
+    python setup.py test -t test -i ${OMERO_DIST}/etc/ice.config -vs -m "not limit_plate"
+else
+    python setup.py test -t test -i ${OMERO_DIST}/etc/ice.config -vs -m limit_plate
+fi

README.md

@@ -70,6 +70,10 @@ Default is `all` and will create the archive. With `none`, only the `transfer.xm
 file is created, in which case the last cli argument is the path where
 the `transfer.xml` file will be written.
 
+`--ignore_errors` gnores any download/export errors during the pack process,
+often the result of servers which do not allow Plate downloads (but will
+ignore any non-zero return code from `omero download` or `omero export`).
+
 
 Examples:
 ```

requirements.txt

@@ -1,3 +1,3 @@
-ezomero>=2.0.0
-ome-types>=0.4.2
+ezomero>=2.0.0<3.0.0
+ome-types>=0.5.0<0.6.0
 setuptools>=58.0.0

setup.py

@@ -84,7 +84,7 @@ def read(fname):
     packages=['', 'omero.plugins'],
     package_dir={"": "src"},
     name="omero-cli-transfer",
-    version='1.0.1',
+    version='1.1.0',
     maintainer="Erick Ratamero",
     maintainer_email="erick.ratamero@jax.org",
     description=("A set of utilities for exporting a transfer"
@@ -96,7 +96,7 @@ def read(fname):
     url="https://github.com/TheJacksonLaboratory/omero-cli-transfer",
     install_requires=[
         'ezomero>=2.1.0, <3.0.0',
-        'ome-types>=0.5.0,<0.6.0'
+        'ome-types==0.5.1.post1'
     ],
     extras_require={
         "rocrate": ["rocrate==0.7.0"],

src/omero_cli_transfer.py

@@ -32,7 +32,7 @@
 from omero.sys import Parameters
 from omero.rtypes import rstring
 from omero.cli import CLI, GraphControl
-from omero.cli import ProxyStringType
+from omero.cli import ProxyStringType, NonZeroReturnCode
 from omero.gateway import BlitzGateway
 from omero.model import Image, Dataset, Project, Plate, Screen
 from omero.grid import ManagedRepositoryPrx as MRepo
@@ -223,6 +223,10 @@ def _configure(self, parser):
         pack.add_argument(
                 "--simple", help="Pack into a human-readable package file",
                 action="store_true")
+        pack.add_argument(
+                "--ignore_errors", help="Ignores any download/export errors "
+                                        "during the pack process",
+                action="store_true")
         pack.add_argument(
             "--metadata",
             choices=['all', 'none', 'img_id', 'timestamp',
@@ -309,7 +313,7 @@ def _get_path_to_repo(self) -> List[str]:
         return mrepos
 
     def _copy_files(self, id_list: Dict[str, Any], folder: str,
-                    conn: BlitzGateway):
+                    ignore_errors: bool, conn: BlitzGateway):
         if not isinstance(id_list, dict):
             raise TypeError("id_list must be a dict")
         if not all(isinstance(item, str) for item in id_list.keys()):
@@ -336,10 +340,34 @@ def _copy_files(self, id_list: Dict[str, Any], folder: str,
                     if rel_path == "pixel_images" or fileset is None:
                         filepath = str(Path(subfolder) /
                                        (str(clean_id) + ".tiff"))
-                        cli.invoke(['export', '--file', filepath, id])
+                        if not ignore_errors:
+                            try:
+                                cli.invoke(['export', '--file', filepath, id],
+                                           strict=True)
+                            except NonZeroReturnCode:
+                                print("A file could not be exported - this is "
+                                      "generally due to a server not allowing"
+                                      " binary downloads.")
+                                shutil.rmtree(folder)
+                                raise NonZeroReturnCode(1, "Download not \
+                                                        allowed")
+                        else:
+                            cli.invoke(['export', '--file', filepath, id])
                         downloaded_ids.append(id)
                     else:
-                        cli.invoke(['download', id, subfolder])
+                        if not ignore_errors:
+                            try:
+                                cli.invoke(['download', id, subfolder],
+                                           strict=True)
+                            except NonZeroReturnCode:
+                                print("A file could not be downloaded - this "
+                                      "is generally due to a server not "
+                                      "allowing binary downloads.")
+                                shutil.rmtree(folder)
+                                raise NonZeroReturnCode(1, "Download not \
+                                                        allowed")
+                        else:
+                            cli.invoke(['download', id, subfolder])
                         for fs_image in fileset.copyImages():
                             downloaded_ids.append(fs_image.getId())
             else:
@@ -349,7 +377,17 @@ def _copy_files(self, id_list: Dict[str, Any], folder: str,
                 ann_folder = str(Path(subfolder).parent)
                 os.makedirs(ann_folder, mode=DIR_PERM, exist_ok=True)
                 id = "File" + id
-                cli.invoke(['download', id, subfolder])
+                if not ignore_errors:
+                    try:
+                        cli.invoke(['download', id, subfolder], strict=True)
+                    except NonZeroReturnCode:
+                        print("A file could not be downloaded - this is "
+                              "generally due to a server not allowing"
+                              " binary downloads.")
+                        shutil.rmtree(folder)
+                        raise NonZeroReturnCode(1, "Download not allowed")
+                else:
+                    cli.invoke(['download', id, subfolder])
 
     def _package_files(self, tar_path: str, zip: bool, folder: str):
         if zip:
@@ -466,10 +504,10 @@ def __pack(self, args):
                                          args.barchive, args.simple,
                                          args.figure,
                                          self.metadata)
-
         if args.binaries == "all":
             print("Starting file copy...")
-            self._copy_files(path_id_dict, folder, self.gateway)
+            self._copy_files(path_id_dict, folder, args.ignore_errors,
+                             self.gateway)
 
         if args.simple:
             self._fix_pixels_image_simple(ome, folder, md_fp)

test/integration/test_transfer.py

@@ -8,6 +8,7 @@
 from omero_cli_transfer import TransferControl
 from cli import CLITest
 from omero.gateway import BlitzGateway
+from omero.cli import NonZeroReturnCode
 
 import ezomero
 import pytest
@@ -17,6 +18,9 @@
 SUPPORTED = [
     "idonly", "imageid", "datasetid", "projectid", "plateid", "screenid"]
 
+PLATESONLY = [
+    "plateid", "screenid"]
+
 TEST_FILES = [
         "test/data/valid_single_image.tar",
         "test/data/valid_single_image.zip",
@@ -149,6 +153,28 @@ def test_pack(self, target_name, tmpdir):
                 self.cli.invoke(args, strict=True)
         self.delete_all()
 
+    @pytest.mark.parametrize('target_name', sorted(PLATESONLY))
+    @pytest.mark.limit_plate
+    def test_pack_noplate(self, target_name, tmpdir):
+        self.create_plate(target_name=target_name)
+        target = getattr(self, target_name)
+        args = self.args + ["pack", target, str(tmpdir / 'test.tar')]
+        with pytest.raises(NonZeroReturnCode):
+            self.cli.invoke(args, strict=True)
+        assert not (os.path.exists(str(tmpdir / 'test.tar')))
+        assert not (os.path.exists(str(tmpdir / 'test.tar_folder')))
+        args = self.args + ["pack", "--binaries", "none", target,
+                            str(tmpdir / 'test.tar')]
+        self.cli.invoke(args, strict=True)
+        assert os.path.exists(str(tmpdir / "test" / "transfer.xml"))
+        assert os.path.getsize(str(tmpdir / "test" / "transfer.xml")) > 0
+        args = self.args + ["pack", "--ignore_errors", target,
+                            str(tmpdir / 'test_ignore.tar')]
+        self.cli.invoke(args, strict=True)
+        assert os.path.exists(str(tmpdir / 'test_ignore.tar'))
+        assert os.path.getsize(str(tmpdir / 'test_ignore.tar')) > 0
+        self.delete_all()
+
     @pytest.mark.parametrize('target_name', sorted(SUPPORTED))
     def test_pack_special(self, target_name, tmpdir):
         if target_name == "datasetid" or target_name == "projectid" or\