Repository: ReproNim/reproman

Issue:
While instantiating resources from the inventory -- guard against parameters read from the file
Should check which are expected from the attr's, and then if there are some additional spurious ones, should filter them out, issue a warning, and then proceed with instantiation using only the "correct" ones.

Discussion:

Patch:
diff --git a/reproman/resource/base.py b/reproman/resource/base.py
index 46c9642..74c0524 100644
--- a/reproman/resource/base.py
+++ b/reproman/resource/base.py
@@ -99,6 +99,62 @@ def get_resource_backends(cls):
             if "doc" in b.metadata}
 
 
+def classify_keys(cls, keys):
+    """Classify `keys` according to the parameters of resource `cls`.
+
+    Parameters
+    ----------
+    cls : Resource object
+    keys : iterable
+       Keys to classify.
+
+    Returns
+    -------
+    A dictionary where each of `keys` is classified into one of the following
+    categories:
+      - required
+        a parameter that must be specified when instantiating the class (either
+        by us or the user)
+      - opt_user
+        a optional parameter that is exposed to the user (i.e. a field that has
+        a default value and "doc" metadata)
+      - opt_internal
+        a unexposed optional parameter (i.e. a field with a default value but
+        with no "doc" metadata) that should not be set by the user (e.g.,
+        status)
+      - unknown
+        a parameter that doesn't fall into any of the above categories
+
+    Raises
+    ------
+    A ResourceError if a required parameter isn't included in `keys`.
+    """
+    all_fields = {f.name for f in attr.fields(cls)}
+    required_params = get_required_fields(cls)
+    required_seen = set()
+    known = get_resource_backends(cls)
+    cats = {"required": [], "opt_user": [], "opt_internal": [], "unknown": []}
+    for key in keys:
+        if key in required_params:
+            required_seen.add(key)
+            cat = "required"
+        elif key in known:
+            cat = "opt_user"
+        elif key in all_fields:
+            # These are attributes like id and status.
+            cat = "opt_internal"
+        else:
+            cat = "unknown"
+        cats[cat].append(key)
+
+    required_missing = required_params.difference(required_seen)
+    if required_missing:
+        raise ResourceError("Missing required backend parameters: {}"
+                            .format(", ".join(sorted(required_missing))))
+
+    return cats
+
+
 def backend_check_parameters(cls, keys):
     """Check whether any backend parameter keys are unknown.
 
@@ -112,14 +168,10 @@ def backend_check_parameters(cls, keys):
     ------
     ResourceError on the first unknown key encountered.
     """
-    required_params = get_required_fields(cls)
-    for req_param in required_params:
-        if req_param not in keys:
-            raise ResourceError(
-                "Missing required backend parameter: " + req_param)
-    known = get_resource_backends(cls)
-    for key in keys:
-        if key not in known and key not in required_params:
+    unknown = classify_keys(cls, keys)["unknown"]
+    if unknown:
+        known = get_resource_backends(cls)
+        for key in unknown:
             if known:
                 import difflib
 
@@ -169,14 +221,44 @@ class ResourceManager(object):
         for r in self.inventory:
             yield r
 
-    @staticmethod
-    def factory(config):
+    def _filter_config(self, cls, config):
+        """Modify `config` to drop invalid parameters for `cls`.
+
+        Parameters
+        ----------
+        cls : Resource object
+        config : dict
+             Configuration parameters for a resource.
+
+        Returns
+        -------
+        A filtered (copy of) config or `config` itself if no modifications were
+        needed.
+        """
+        unknown = classify_keys(cls, config)["unknown"]
+        if unknown:
+            config = config.copy()
+            inv_config = self.inventory.get(config["name"], {})
+            for unk_param in unknown:
+                msg_extra = ""
+                if unk_param in inv_config:
+                    msg_extra = (". Consider removing it from {}"
+                                 .format(self._inventory_path))
+                lgr.warning("%s is not a known %s parameter%s",
+                            unk_param, config["type"], msg_extra)
+                config.pop(unk_param)
+        return config
+
+    def factory(self, config, strict=True):
         """Factory method for creating the appropriate Container sub-class.
 
         Parameters
         ----------
         config : dict
             Configuration parameters for the resource.
+        strict : optional
+            When set to false, drop unknown keys from `config`, after issuing a
+            warning, rather than raising a ResourceError.
 
         Returns
         -------
@@ -187,12 +269,20 @@ class ResourceManager(object):
 
         type_ = config['type']
         cls = get_resource_class(type_)
+
+        if not strict:
+            config = self._filter_config(cls, config)
+
         try:
             instance = cls(**config)
         except TypeError:
-            backend_check_parameters(cls, config)
-            # The check didn't raise an exception, so this wasn't related to an
-            # unknown backend parameter.
+            if strict:
+                # In strict mode, the exception may be due to an unknown
+                # backend paraemter. Call backend_check_parameters() to get a
+                # more meaningful ResourceError.
+                backend_check_parameters(cls, config)
+                # Never mind. The check didn't raise an exception, so this
+                # wasn't related to an unknown backend parameter.
             raise
         return instance
 
@@ -272,7 +362,8 @@ class ResourceManager(object):
         -------
         A Resource object
         """
-        return self.factory(self._get_resource_config(resref, resref_type))
+        return self.factory(self._get_resource_config(resref, resref_type),
+                            strict=False)
 
     def _get_inventory(self):
         """Return a dict with the config information for all resources.


Test patch:
diff --git a/reproman/conftest.py b/reproman/conftest.py
index 1b89f70..c53e848 100644
--- a/reproman/conftest.py
+++ b/reproman/conftest.py
@@ -11,6 +11,7 @@
 # Some commonly used fixtures
 
 from reproman.formats.tests.fixtures import demo1_spec, reprozip_spec2
+from reproman.tests.fixtures import resource_manager_fixture
 
 import pytest
 
@@ -29,3 +30,9 @@ def pytest_collection_modifyitems(config, items):
     for item in items:
         if "integration" in item.keywords:
             item.add_marker(skip_integration)
+
+
+# This is useful for tests that need a ResourceManager instance but do not need
+# any resources and don't plan on modifying the on-disk inventory. If you do
+# need to modify the resources, use `resource_manager_fixture` directly.
+resman = resource_manager_fixture(resources={}, scope="session")
diff --git a/reproman/resource/tests/test_aws_ec2.py b/reproman/resource/tests/test_aws_ec2.py
index 5289f9d..3508019 100644
--- a/reproman/resource/tests/test_aws_ec2.py
+++ b/reproman/resource/tests/test_aws_ec2.py
@@ -12,13 +12,12 @@ from unittest.mock import patch, call, MagicMock
 from ...utils import swallow_logs
 from ...tests.utils import assert_in
 from ...tests.skip import mark
-from ..base import ResourceManager
 from ..ssh import SSH
 
 pytestmark = mark.skipif_no_aws_dependencies
 
 
-def test_awsec2_class():
+def test_awsec2_class(resman):
 
     with patch('boto3.resource') as client, \
             patch.object(SSH, 'get_session', return_value='started_session'), \
@@ -34,7 +33,7 @@ def test_awsec2_class():
             'access_key_id': 'my-aws-access-key-id',
             'secret_access_key': 'my-aws-secret-access-key-id'
         }
-        resource = ResourceManager.factory(config)
+        resource = resman.factory(config)
         resource.connect()
         assert resource.id is None
         assert resource.status is None
@@ -58,7 +57,7 @@ def test_awsec2_class():
             'access_key_id': 'my-aws-access-key-id',
             'secret_access_key': 'my-aws-secret-access-key-id'
         }
-        resource = ResourceManager.factory(config)
+        resource = resman.factory(config)
         try:
             resource.connect()
         except Exception as e:
@@ -80,7 +79,7 @@ def test_awsec2_class():
             'key_name': 'my-ssh-key',
             'key_filename': '/home/me/.ssh/id_rsa'
         }
-        resource = ResourceManager.factory(config)
+        resource = resman.factory(config)
         resource.connect()
         assert resource.image == 'ami-c8580bdf'
         assert resource.id == 'i-00002777d52482d9c'
@@ -117,7 +116,7 @@ def test_awsec2_class():
             'key_name': 'my-ssh-key',
             'key_filename': '/home/me/.ssh/id_rsa'
         }
-        resource = ResourceManager.factory(config)
+        resource = resman.factory(config)
         resource.connect()
         # Test retrieving more than one yield from the create method
         create_generator = resource.create()
diff --git a/reproman/resource/tests/test_base.py b/reproman/resource/tests/test_base.py
index e406261..7fd80d7 100644
--- a/reproman/resource/tests/test_base.py
+++ b/reproman/resource/tests/test_base.py
@@ -11,6 +11,7 @@ import pytest
 
 from reproman.config import ConfigManager
 from reproman.resource.base import ResourceManager
+from reproman.resource.base import Resource
 from reproman.resource.base import backend_check_parameters
 from reproman.resource.base import get_resource_class
 from reproman.resource.shell import Shell
@@ -21,14 +22,37 @@ from reproman.support.exceptions import ResourceError
 from reproman.tests.skip import mark
 
 
-def test_resource_manager_factory_missing_type():
+def test_resource_manager_factory_missing_type(resman):
     with pytest.raises(MissingConfigError):
-        ResourceManager.factory({})
+        resman.factory({})
 
 
-def test_resource_manager_factory_unkown():
+def test_resource_manager_factory_unkown(resman):
     with pytest.raises(ResourceError):
-        ResourceManager.factory({"type": "not really a type"})
+        resman.factory({"type": "not really a type"})
+
+
+def test_resource_manager_factory_missing_required(resman):
+    with pytest.raises(ResourceError):
+        resman.factory({"type": "shell"})
+
+
+@pytest.mark.parametrize("type_", ["shell", "ssh"])
+def test_resource_manager_factory_invalid_param(resman, type_):
+    config = {"type": type_,
+              "id": "id",
+              "name": "name",
+              # All of the below are invalid in the case of shell. For ssh,
+              # "other" is invalid.
+              "host": "host",
+              "port": "port",
+              "other": "doesntmatter"}
+
+    with pytest.raises(ResourceError):
+        resman.factory(config)
+
+    res = resman.factory(config, strict=False)
+    assert isinstance(res, Resource)
 
 
 def test_backend_check_parameters_no_known():
diff --git a/reproman/resource/tests/test_docker_container.py b/reproman/resource/tests/test_docker_container.py
index 98df863..3be21a5 100644
--- a/reproman/resource/tests/test_docker_container.py
+++ b/reproman/resource/tests/test_docker_container.py
@@ -13,7 +13,6 @@ from ...utils import merge_dicts
 from ...utils import swallow_logs
 from ...tests.utils import assert_in
 from ...tests.skip import mark
-from ..base import ResourceManager
 from ...support.exceptions import ResourceError
 from ...consts import TEST_SSH_DOCKER_DIGEST
 
@@ -30,7 +29,7 @@ setup_ubuntu = get_docker_fixture(
 
 
 @mark.skipif_no_docker_dependencies
-def test_dockercontainer_class():
+def test_dockercontainer_class(resman):
 
     with patch('docker.Client') as client, \
         patch('dockerpty.start') as dockerpty, \
@@ -74,7 +73,7 @@ def test_dockercontainer_class():
             'name': 'non-existent-resource',
             'type': 'docker-container'
         }
-        resource = ResourceManager.factory(config)
+        resource = resman.factory(config)
         resource.connect()
         assert resource.id is None
         assert resource.status is None
@@ -84,7 +83,7 @@ def test_dockercontainer_class():
             'name': 'duplicate-resource-name',
             'type': 'docker-container'
         }
-        resource = ResourceManager.factory(config)
+        resource = resman.factory(config)
         with raises(ResourceError) as ecm:
             resource.connect()
         assert ecm.value.args[0].startswith("Multiple container matches found")
@@ -96,7 +95,7 @@ def test_dockercontainer_class():
             'engine_url': 'tcp://127.0.0.1:2375',
             'seccomp_unconfined': True
         }
-        resource = ResourceManager.factory(config)
+        resource = resman.factory(config)
         resource.connect()
         assert resource.image == 'ubuntu:latest'
         assert resource.engine_url == 'tcp://127.0.0.1:2375'
@@ -117,7 +116,7 @@ def test_dockercontainer_class():
             'type': 'docker-container',
             'engine_url': 'tcp://127.0.0.1:2375'
         }
-        resource = ResourceManager.factory(config)
+        resource = resman.factory(config)
         resource.connect()
         results = merge_dicts(resource.create())
         assert results['id'] == '18b31b30e3a5'
diff --git a/reproman/resource/tests/test_shell.py b/reproman/resource/tests/test_shell.py
index d9250cf..d782af0 100644
--- a/reproman/resource/tests/test_shell.py
+++ b/reproman/resource/tests/test_shell.py
@@ -17,13 +17,12 @@ from unittest.mock import patch, call
 from ...utils import merge_dicts
 from ...utils import swallow_logs
 from ...tests.utils import assert_in
-from ..base import ResourceManager
 from ...cmd import Runner
 from ..shell import Shell, ShellSession
 from .test_session import check_session_passing_envvars
 
 
-def test_shell_class():
+def test_shell_class(resman):
 
     with patch.object(Runner, 'run', return_value='installed package') as runner, \
             swallow_logs(new_level=logging.DEBUG) as log:
@@ -33,7 +32,7 @@ def test_shell_class():
             'name': 'my-shell',
             'type': 'shell'
         }
-        shell = ResourceManager.factory(config)
+        shell = resman.factory(config)
 
         command = ['apt-get', 'install', 'bc']
         shell.add_command(command)
@@ -127,13 +126,13 @@ def test_session_passing_envvars():
     check_session_passing_envvars(ShellSession())
 
 
-def test_shell_resource():
+def test_shell_resource(resman):
 
     config = {
         'name': 'test-ssh-resource',
         'type': 'shell'
     }
-    resource = ResourceManager.factory(config)
+    resource = resman.factory(config)
 
     status = merge_dicts(resource.create())
     assert re.match('\w{8}-\w{4}-\w{4}-\w{4}-\w{12}$', status['id']) is not None
diff --git a/reproman/resource/tests/test_ssh.py b/reproman/resource/tests/test_ssh.py
index 007c442..3e577eb 100644
--- a/reproman/resource/tests/test_ssh.py
+++ b/reproman/resource/tests/test_ssh.py
@@ -20,7 +20,6 @@ from ...utils import merge_dicts
 from ...utils import swallow_logs
 from ...tests.utils import assert_in
 from ...tests.skip import mark
-from ..base import ResourceManager
 from reproman.tests.fixtures import get_docker_fixture
 from ...consts import TEST_SSH_DOCKER_DIGEST
 
@@ -48,7 +47,7 @@ def test_setup_ssh(setup_ssh):
     assert setup_ssh['custom']['host'] == 'localhost'
 
 
-def test_ssh_class(setup_ssh, resource_test_dir):
+def test_ssh_class(setup_ssh, resource_test_dir, resman):
     with swallow_logs(new_level=logging.DEBUG) as log:
 
         # Test connecting to test SSH server.
@@ -58,7 +57,7 @@ def test_ssh_class(setup_ssh, resource_test_dir):
             type='ssh',
             **setup_ssh['custom']
         )
-        resource = ResourceManager.factory(config)
+        resource = resman.factory(config)
         updated_config = merge_dicts(resource.create())
         config.update(updated_config)
         assert re.match('\w{8}-\w{4}-\w{4}-\w{4}-\w{12}$',
@@ -116,7 +115,7 @@ def test_ssh_class(setup_ssh, resource_test_dir):
             session._execute_command('non-existent-command', cwd='/path')
 
 
-def test_ssh_resource(setup_ssh):
+def test_ssh_resource(setup_ssh, resman):
 
     config = {
         'name': 'ssh-test-resource',
@@ -125,7 +124,7 @@ def test_ssh_resource(setup_ssh):
         'user': 'root',
         'port': 49000
     }
-    resource = ResourceManager.factory(config)
+    resource = resman.factory(config)
     resource.connect(password='root')
 
     with pytest.raises(NotImplementedError):