21020: Make arv-copy search from environment
authorBrett Smith <brett.smith@curii.com>
Mon, 20 May 2024 20:26:21 +0000 (16:26 -0400)
committerBrett Smith <brett.smith@curii.com>
Fri, 24 May 2024 21:02:45 +0000 (17:02 -0400)
Arvados-DCO-1.1-Signed-off-by: Brett Smith <brett.smith@curii.com>

sdk/python/arvados/commands/arv_copy.py
sdk/python/tests/test_arv_copy.py

index eccf488efb4dca5bfadb5613336d55035238c7fa..d8efa225569dab67b16fda70c32c7b562e1001cb 100755 (executable)
@@ -13,9 +13,9 @@
 # --no-recursive is given, arv-copy copies only the single record
 # identified by object-uuid.
 #
-# The user must have files $HOME/.config/arvados/{src}.conf and
-# $HOME/.config/arvados/{dst}.conf with valid login credentials for
-# instances src and dst.  If either of these files is not found,
+# The user must have configuration files {src}.conf and
+# {dst}.conf in a standard configuration directory with valid login credentials
+# for instances src and dst.  If either of these files is not found,
 # arv-copy will issue an error.
 
 import argparse
@@ -86,10 +86,10 @@ def main():
         help='Perform copy even if the object appears to exist at the remote destination.')
     copy_opts.add_argument(
         '--src', dest='source_arvados',
-        help='The cluster id of the source Arvados instance. May be either a pathname to a config file, or (for example) "foo" as shorthand for $HOME/.config/arvados/foo.conf.  If not provided, will be inferred from the UUID of the object being copied.')
+        help='The cluster id of the source Arvados instance. May be either a pathname to a config file, or (for example) "foo" as shorthand for finding "foo.conf" under a systemd or XDG configuration directory.  If not provided, will be inferred from the UUID of the object being copied.')
     copy_opts.add_argument(
         '--dst', dest='destination_arvados',
-        help='The name of the destination Arvados instance (required). May be either a pathname to a config file, or (for example) "foo" as shorthand for $HOME/.config/arvados/foo.conf.  If not provided, will use ARVADOS_API_HOST from environment.')
+        help='The name of the destination Arvados instance. May be either a pathname to a config file, or (for example) "foo" as shorthand for finding "foo.conf" under a systemd or XDG configuration directory.  If not provided, will use ARVADOS_API_HOST from environment.')
     copy_opts.add_argument(
         '--recursive', dest='recursive', action='store_true',
         help='Recursively copy any dependencies for this object, and subprojects. (default)')
@@ -197,8 +197,8 @@ def set_src_owner_uuid(resource, uuid, args):
 #     (either local or absolute) to a file with Arvados configuration
 #     settings.
 #
-#     Otherwise, it is presumed to be the name of a file in
-#     $HOME/.config/arvados/instance_name.conf
+#     Otherwise, it is presumed to be the name of a file in a standard
+#     configuration directory.
 #
 def api_for_instance(instance_name, num_retries):
     if not instance_name:
@@ -208,16 +208,22 @@ def api_for_instance(instance_name, num_retries):
     if '/' in instance_name:
         config_file = instance_name
     else:
-        config_file = os.path.join(os.environ['HOME'], '.config', 'arvados', "{}.conf".format(instance_name))
+        dirs = arvados.util._BaseDirectories('CONFIG')
+        config_file = next(dirs.search(f'{instance_name}.conf'), '')
 
     try:
         cfg = arvados.config.load(config_file)
-    except (IOError, OSError) as e:
-        abort(("Could not open config file {}: {}\n" +
+    except OSError as e:
+        if config_file:
+            verb = 'open'
+        else:
+            verb = 'find'
+            config_file = f'{instance_name}.conf'
+        abort(("Could not {} config file {}: {}\n" +
                "You must make sure that your configuration tokens\n" +
                "for Arvados instance {} are in {} and that this\n" +
                "file is readable.").format(
-                   config_file, e, instance_name, config_file))
+                   verb, config_file, e.strerror, instance_name, config_file))
 
     if 'ARVADOS_API_HOST' in cfg and 'ARVADOS_API_TOKEN' in cfg:
         api_is_insecure = (
index 1af5c68e6c94934b57364b33aaacc4ee35307d2e..30076291210c37a5eb7d938fb72b62627de97dc7 100644 (file)
@@ -2,14 +2,18 @@
 #
 # SPDX-License-Identifier: Apache-2.0
 
+import itertools
 import os
 import sys
 import tempfile
 import unittest
 import shutil
 import arvados.api
+import arvados.util
 from arvados.collection import Collection, CollectionReader
 
+import pytest
+
 import arvados.commands.arv_copy as arv_copy
 from . import arvados_testutil as tutil
 from . import run_test_server
@@ -87,3 +91,69 @@ class ArvCopyVersionTestCase(run_test_server.TestCaseWithServers, tutil.VersionC
         finally:
             os.environ['HOME'] = home_was
             shutil.rmtree(tmphome)
+
+
+class TestApiForInstance:
+    _token_counter = itertools.count(1)
+
+    @staticmethod
+    def api_config(version, **kwargs):
+        assert version == 'v1'
+        return kwargs
+
+    @pytest.fixture
+    def patch_api(self, monkeypatch):
+        monkeypatch.setattr(arvados, 'api', self.api_config)
+
+    @pytest.fixture
+    def config_file(self, tmp_path):
+        count = next(self._token_counter)
+        path = tmp_path / f'config{count}.conf'
+        with path.open('w') as config_file:
+            print(
+                "ARVADOS_API_HOST=localhost",
+                f"ARVADOS_API_TOKEN={self.expected_token(path)}",
+                sep="\n", file=config_file,
+            )
+        return path
+
+    @pytest.fixture
+    def patch_search(self, tmp_path, monkeypatch):
+        def search(self, name):
+            path = tmp_path / name
+            if path.exists():
+                yield path
+        monkeypatch.setattr(arvados.util._BaseDirectories, 'search', search)
+
+    def expected_token(self, path):
+        return f"v2/zzzzz-gj3su-{path.stem:>015s}/{path.stem:>050s}"
+
+    def test_from_environ(self, patch_api):
+        actual = arv_copy.api_for_instance('', 0)
+        assert actual == {}
+
+    def test_relative_path(self, patch_api, config_file, monkeypatch):
+        monkeypatch.chdir(config_file.parent)
+        actual = arv_copy.api_for_instance(f'./{config_file.name}', 0)
+        assert actual['host'] == 'localhost'
+        assert actual['token'] == self.expected_token(config_file)
+
+    def test_absolute_path(self, patch_api, config_file):
+        actual = arv_copy.api_for_instance(str(config_file), 0)
+        assert actual['host'] == 'localhost'
+        assert actual['token'] == self.expected_token(config_file)
+
+    def test_search_path(self, patch_api, patch_search, config_file):
+        actual = arv_copy.api_for_instance(config_file.stem, 0)
+        assert actual['host'] == 'localhost'
+        assert actual['token'] == self.expected_token(config_file)
+
+    def test_search_failed(self, patch_api, patch_search):
+        with pytest.raises(SystemExit) as exc_info:
+            arv_copy.api_for_instance('NotFound', 0)
+        assert exc_info.value.code > 0
+
+    def test_path_unreadable(self, patch_api, tmp_path):
+        with pytest.raises(SystemExit) as exc_info:
+            arv_copy.api_for_instance(str(tmp_path / 'nonexistent.conf'), 0)
+        assert exc_info.value.code > 0