Repository: ReproNim/reproman

Issue:
Add --force to delete to allow remove resource even when actual instance already gone
ATM attempt to remove a resource which requires also removal of some instance (cloud instance, etc) would fail if that thing is already remove through other means.  I think we need `--force` option which proceed removing everything it can even if some component (docker container, cloud instance etc) already cannot be found 

```
$> niceman delete test-ec2-west      
Delete the resource 'test-ec2-west'? (ID: i-0368a48188f646b34) (choices: yes, [no], y, n): y

2018-12-18 17:06:25,332 [ERROR  ] An error occurred (InvalidInstanceID.NotFound) when calling the DescribeInstances operation: The instance ID 'i-0368a48188f646b34' does not exist [client.py:_make_api_call:624] (ClientError) 

$> niceman delete test-ec2-west
Delete the resource 'test-ec2-west'? (ID: i-0368a48188f646b34) (choices: yes, [no], y, n): y

2018-12-18 17:06:33,837 [ERROR  ] An error occurred (InvalidInstanceID.NotFound) when calling the DescribeInstances operation: The instance ID 'i-0368a48188f646b34' does not exist [client.py:_make_api_call:624] (ClientError) 
```

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/ReproNim/reproman/pull/370?src=pr&el=h1) Report
> Merging [#370](https://codecov.io/gh/ReproNim/reproman/pull/370?src=pr&el=desc) into [master](https://codecov.io/gh/ReproNim/reproman/commit/b72353753c4369745993d101dece02f6172006b6?src=pr&el=desc) will **increase** coverage by `0.04%`.
> The diff coverage is `95%`.

[![Impacted file tree graph](https://codecov.io/gh/ReproNim/reproman/pull/370/graphs/tree.svg?width=650&token=4i2BaUgDTT&height=150&src=pr)](https://codecov.io/gh/ReproNim/reproman/pull/370?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
+ Coverage   89.51%   89.56%   +0.04%     
==========================================
  Files         137      137              
  Lines       10302    10284      -18     
==========================================
- Hits         9222     9211      -11     
+ Misses       1080     1073       -7
```


| [Impacted Files](https://codecov.io/gh/ReproNim/reproman/pull/370?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [reproman/tests/test\_utils.py](https://codecov.io/gh/ReproNim/reproman/pull/370/diff?src=pr&el=tree#diff-cmVwcm9tYW4vdGVzdHMvdGVzdF91dGlscy5weQ==) | `94.05% <ø> (-0.13%)` | :arrow_down: |
| [reproman/interface/tests/test\_create.py](https://codecov.io/gh/ReproNim/reproman/pull/370/diff?src=pr&el=tree#diff-cmVwcm9tYW4vaW50ZXJmYWNlL3Rlc3RzL3Rlc3RfY3JlYXRlLnB5) | `100% <ø> (ø)` | :arrow_up: |
| [reproman/utils.py](https://codecov.io/gh/ReproNim/reproman/pull/370/diff?src=pr&el=tree#diff-cmVwcm9tYW4vdXRpbHMucHk=) | `86.2% <ø> (-0.13%)` | :arrow_down: |
| [reproman/resource/ssh.py](https://codecov.io/gh/ReproNim/reproman/pull/370/diff?src=pr&el=tree#diff-cmVwcm9tYW4vcmVzb3VyY2Uvc3NoLnB5) | `85.84% <100%> (+0.13%)` | :arrow_up: |
| [reproman/resource/tests/test\_singularity.py](https://codecov.io/gh/ReproNim/reproman/pull/370/diff?src=pr&el=tree#diff-cmVwcm9tYW4vcmVzb3VyY2UvdGVzdHMvdGVzdF9zaW5ndWxhcml0eS5weQ==) | `100% <100%> (ø)` | :arrow_up: |
| [reproman/resource/tests/test\_docker\_container.py](https://codecov.io/gh/ReproNim/reproman/pull/370/diff?src=pr&el=tree#diff-cmVwcm9tYW4vcmVzb3VyY2UvdGVzdHMvdGVzdF9kb2NrZXJfY29udGFpbmVyLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [reproman/resource/tests/test\_base.py](https://codecov.io/gh/ReproNim/reproman/pull/370/diff?src=pr&el=tree#diff-cmVwcm9tYW4vcmVzb3VyY2UvdGVzdHMvdGVzdF9iYXNlLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [reproman/resource/docker\_container.py](https://codecov.io/gh/ReproNim/reproman/pull/370/diff?src=pr&el=tree#diff-cmVwcm9tYW4vcmVzb3VyY2UvZG9ja2VyX2NvbnRhaW5lci5weQ==) | `95.33% <100%> (ø)` | :arrow_up: |
| [reproman/resource/singularity.py](https://codecov.io/gh/ReproNim/reproman/pull/370/diff?src=pr&el=tree#diff-cmVwcm9tYW4vcmVzb3VyY2Uvc2luZ3VsYXJpdHkucHk=) | `89.62% <100%> (ø)` | :arrow_up: |
| [reproman/resource/tests/test\_session.py](https://codecov.io/gh/ReproNim/reproman/pull/370/diff?src=pr&el=tree#diff-cmVwcm9tYW4vcmVzb3VyY2UvdGVzdHMvdGVzdF9zZXNzaW9uLnB5) | `99.53% <100%> (ø)` | :arrow_up: |
| ... and [14 more](https://codecov.io/gh/ReproNim/reproman/pull/370/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/ReproNim/reproman/pull/370?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/ReproNim/reproman/pull/370?src=pr&el=footer). Last update [b723537...48b6e3a](https://codecov.io/gh/ReproNim/reproman/pull/370?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/reproman/interface/delete.py b/reproman/interface/delete.py
index 72002be..c11a16f 100644
--- a/reproman/interface/delete.py
+++ b/reproman/interface/delete.py
@@ -11,12 +11,10 @@
 
 __docformat__ = 'restructuredtext'
 
-import re
-
 from .base import Interface
-import reproman.interface.base # Needed for test patching
 from .common_opts import resref_arg
 from .common_opts import resref_type_opt
+from ..dochelpers import exc_str
 from ..support.param import Parameter
 from ..support.constraints import EnsureStr
 from ..resource import get_manager
@@ -45,29 +43,46 @@ class Delete(Interface):
         # ),
         resref_type=resref_type_opt,
         skip_confirmation=Parameter(
-            args=("--skip-confirmation",),
+            args=("-y", "--skip-confirmation",),
             action="store_true",
             doc="Delete resource without prompting user for confirmation",
         ),
+        force=Parameter(
+            args=("-f", "--force"),
+            action="store_true",
+            doc="""Remove a resource from the local inventory regardless of
+            connection errors. Use with caution!""",
+        ),
     )
 
     @staticmethod
-    def __call__(resref, resref_type="auto", skip_confirmation=False):
+    def __call__(resref, resref_type="auto", skip_confirmation=False,
+                 force=False):
+
         from reproman.ui import ui
 
         manager = get_manager()
         resource = manager.get_resource(resref, resref_type)
 
         if skip_confirmation:
-            response = True
+            delete_confirmed = True
         else:
-            response = ui.yesno(
+            delete_confirmed = ui.yesno(
                 "Delete the resource '{}'? (ID: {})".format(
                     resource.name, resource.id[:20]),
                 default="no"
             )
 
-        if response:
-            resource.connect()
-            manager.delete(resource)
+        if delete_confirmed:
+            try:
+                resource.connect()
+                manager.delete(resource)
+            except Exception as exc:
+                if force:
+                    lgr.warning("Force deleting %s following failure: %s",
+                                resource.name, exc_str(exc))
+                    manager.delete(resource, inventory_only=True)
+                else:
+                    raise
+
             lgr.info("Deleted the environment %s", resource.name)
diff --git a/reproman/interface/ls.py b/reproman/interface/ls.py
index 93c1451..6e1884f 100644
--- a/reproman/interface/ls.py
+++ b/reproman/interface/ls.py
@@ -11,16 +11,12 @@
 
 __docformat__ = 'restructuredtext'
 
-from configparser import NoSectionError
-
 from .base import Interface
-import reproman.interface.base # Needed for test patching
+# import reproman.interface.base  # Needed for test patching
 from ..support.param import Parameter
-from ..support.constraints import EnsureStr, EnsureNone
-from  ..resource import get_manager
+from ..resource import get_manager
 from ..ui import ui
 from ..support.exceptions import ResourceError
-from ..support.exceptions import ResourceNotFoundError
 from ..dochelpers import exc_str
 
 from logging import getLogger
@@ -40,15 +36,12 @@ class Ls(Interface):
         verbose=Parameter(
             args=("-v", "--verbose"),
             action="store_true",
-            #constraints=EnsureBool() | EnsureNone(),
             doc="provide more verbose listing",
         ),
         refresh=Parameter(
-            args=("--refresh",),
+            args=("-r", "--refresh",),
             action="store_true",
             doc="Refresh the status of the resources listed",
-            # metavar='CONFIG',
-            # constraints=EnsureStr(),
         ),
     )
 
@@ -64,38 +57,33 @@ class Ls(Interface):
             if name.startswith('_'):
                 continue
 
-            # if refresh:
             try:
-                resource = manager.get_resource(name, resref_type="name")
-            except ResourceNotFoundError:
-                lgr.warning("Manager did not return a resource for %r", name)
+                resource = manager.get_resource(manager.inventory[name]['id'])
+            except ResourceError as e:
+                lgr.warning("Manager did not return a resource for %s: %s", name, exc_str(e))
                 continue
 
-            try:
-                if refresh:
+            if refresh:
+                try:
                     resource.connect()
-                if not resource.id:
-                    # continue  # A missing ID indicates a deleted resource.
-                    resource.id = 'DELETED'
-                    resource.status = 'N/A'
-                report_status = resource.status
-            except Exception as exc:
-                lgr.error("%s resource query error: %s", name, exc_str(exc))
-                report_status = "N/A (QUERY-ERROR)"
-                for f in 'id', 'status':
-                    if not getattr(resource, f):
-                        setattr(resource, f, "?")
+                    if not resource.id:
+                        resource.status = 'NOT FOUND'
+                except Exception as e:
+                    lgr.debug("%s resource query error: %s", name, exc_str(e))
+                    resource.status = 'CONNECTION ERROR'
+
+                manager.inventory[name].update({'status': resource.status})
+
             msgargs = (
                 name,
                 resource.type,
-                resource.id[:id_length] if resource.id else '',
-                report_status,
+                manager.inventory[name]['id'][:id_length],
+                resource.status,
             )
             ui.message(template.format(*msgargs))
             lgr.debug('list result: {}, {}, {}, {}'.format(*msgargs))
 
-        # if not refresh:
-        #     ui.message('(Use --refresh option to view current status.)')
-        #
-        # if refresh:
-        #     reproman.interface.base.set_resource_inventory(inventory)
+        if refresh:
+            manager.save_inventory()
+        else:
+            ui.message('Use --refresh option to view updated status.')
diff --git a/reproman/resource/base.py b/reproman/resource/base.py
index 5c7f0cf..32310fc 100644
--- a/reproman/resource/base.py
+++ b/reproman/resource/base.py
@@ -280,13 +280,13 @@ class ResourceManager(object):
 
         return inventory
 
-    def _save(self):
+    def save_inventory(self):
         """Save the resource inventory.
         """
         # Operate on a copy so there is no side-effect of modifying original
         # inventory.
         #
-        # The attribute may not exist yet because _get_inventory calls _save.
+        # The attribute may not exist yet because _get_inventory calls save_inventory.
         inventory = self.inventory.copy() if hasattr(self, "inventory") else {}
 
         for key in list(inventory):  # go through a copy of all keys since we modify
@@ -336,18 +336,23 @@ class ResourceManager(object):
         for resource_attrs in resource.create():
             config.update(resource_attrs)
             self.inventory[name] = config
-            self._save()
+            self.save_inventory()
 
-    def delete(self, resource):
+    def delete(self, resource, inventory_only=False):
         """Delete `resource` from the inventory.
 
         Parameters
         ----------
         resource : Resource object
+
+        inventory_only : boolean
+            Flag to indicate that we should only remove the resource record
+            from the inventory file
         """
-        resource.delete()
+        if not inventory_only:
+            resource.delete()
         del self.inventory[resource.name]
-        self._save()
+        self.save_inventory()
 
     def start(self, resource):
         """Start a `resource` in the inventory.
@@ -363,7 +368,7 @@ class ResourceManager(object):
             return
 
         self.inventory[resource.name]['status'] = "running"
-        self._save()
+        self.save_inventory()
         lgr.info("Started the environment %s (%s)", resource.name, resource.id)
 
     def stop(self, resource):
@@ -380,7 +385,7 @@ class ResourceManager(object):
             return
 
         self.inventory[resource.name]['status'] = "stopped"
-        self._save()
+        self.save_inventory()
         lgr.info("Stopped the environment %s", resource.name)
 
 


Test patch:
diff --git a/reproman/interface/tests/test_create.py b/reproman/interface/tests/test_create.py
index 004c16f..bcf64bf 100644
--- a/reproman/interface/tests/test_create.py
+++ b/reproman/interface/tests/test_create.py
@@ -29,7 +29,7 @@ def test_create_interface():
     """
 
     with patch('docker.Client') as client, \
-        patch('reproman.resource.ResourceManager._save'), \
+        patch('reproman.resource.ResourceManager.save_inventory'), \
         patch('reproman.resource.ResourceManager._get_inventory'), \
         swallow_logs(new_level=logging.DEBUG) as log:
 
diff --git a/reproman/interface/tests/test_delete.py b/reproman/interface/tests/test_delete.py
index 5ebe320..6239a40 100644
--- a/reproman/interface/tests/test_delete.py
+++ b/reproman/interface/tests/test_delete.py
@@ -12,9 +12,48 @@ from reproman.cmdline.main import main
 import logging
 from mock import patch, call, MagicMock
 
-from reproman.utils import swallow_logs
-from reproman.resource.base import ResourceManager
-from reproman.tests.utils import assert_in
+from ...resource.base import ResourceManager
+from ...utils import swallow_logs
+from ...tests.utils import assert_in
+
+
+def mock_get_manager():
+    manager = ResourceManager()
+    manager.inventory = {
+        "my-resource": {
+            "status": "running",
+            "engine_url": "tcp://127.0.0.1:2375",
+            "type": "docker-container",
+            "name": "my-resource",
+            "id": "326b0fdfbf838"
+        },
+        "missing-resource": {
+            'id': 'i-22221ddf096c22bb0',
+            'type': 'aws-ec2',
+            'access_key_id': 'my-aws-access-key-id',
+            'secret_access_key': 'my-aws-secret-access-key-id',
+            'key_name': 'my-ssh-key',
+            'key_filename': '/home/me/.ssh/id_rsa',
+            'status': 'running',
+            'name': 'missing-resource'
+        }
+    }
+    manager.save_inventory = lambda: None  # Don't save test data to file system.
+    return manager
+
+
+def mock_docker_client():
+    mock = MagicMock()
+    mock.return_value = MagicMock(
+        containers=lambda all: [
+            {
+                'Id': '326b0fdfbf838',
+                'Names': ['/my-resource'],
+                'State': 'running'
+            }
+        ]
+    )
+    return mock
 
 
 def test_delete_interface():
@@ -22,40 +61,12 @@ def test_delete_interface():
     Test deleting a resource.
     """
 
-    with patch('docker.Client') as client, \
-        patch('reproman.resource.ResourceManager._save'), \
-        patch('reproman.resource.ResourceManager._get_inventory') as get_inventory, \
-        swallow_logs(new_level=logging.DEBUG) as log:
-
-        client.return_value = MagicMock(
-            containers=lambda all: [
-                {
-                    'Id': '326b0fdfbf838',
-                    'Names': ['/my-resource'],
-                    'State': 'running'
-                }
-            ]
-        )
-
-        get_inventory.return_value = {
-            "my-resource": {
-                "status": "running",
-                "engine_url": "tcp://127.0.0.1:2375",
-                "type": "docker-container",
-                "name": "my-resource",
-                "id": "326b0fdfbf838"
-            }
-        }
-
-        args = [
-            'delete',
-            '--skip-confirmation',
-            'my-resource'
+    with patch('docker.Client', new_callable=mock_docker_client) as client, \
+            patch('reproman.interface.delete.get_manager',
+                new=mock_get_manager), \
+            swallow_logs(new_level=logging.DEBUG) as log:
 
-        ]
-        with patch("reproman.interface.delete.get_manager",
-                   return_value=ResourceManager()):
-            main(args)
+        main(['delete', '--skip-confirmation', 'my-resource'])
 
         calls = [
             call(base_url='tcp://127.0.0.1:2375'),
@@ -69,5 +80,7 @@ def test_delete_interface():
             )
         ]
         client.assert_has_calls(calls, any_order=True)
-
-        assert_in('Deleted the environment my-resource', log.lines)
\ No newline at end of file
+        assert_in('Deleted the environment my-resource', log.lines)
+   
+        main(['delete', '-y', '--force', 'missing-resource'])
+        assert_in('Deleted the environment missing-resource', log.lines)
diff --git a/reproman/interface/tests/test_ls.py b/reproman/interface/tests/test_ls.py
index 07796b0..d5f77ed 100644
--- a/reproman/interface/tests/test_ls.py
+++ b/reproman/interface/tests/test_ls.py
@@ -7,86 +7,67 @@
 #
 # ## ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ##
 
-from mock import patch, MagicMock
+from mock import patch
 
+from ...resource.base import ResourceManager
 from ...cmdline.main import main
 from ...utils import swallow_logs
-from ...resource.base import ResourceManager
 from ...tests.utils import assert_in
 
 import logging
 
 
+def mock_get_manager():
+    manager = ResourceManager()
+    manager.inventory = {
+        'docker-resource-1': {
+            'status': 'running',
+            'engine_url': 'tcp://127.0.0.1:2375',
+            'type': 'docker-container',
+            'name': 'docker-resource-1',
+            'id': '326b0fdfbf838'
+        },
+        'ec2-resource-1': {
+            'id': 'i-22221ddf096c22bb0',
+            'type': 'aws-ec2',
+            'access_key_id': 'my-aws-access-key-id',
+            'secret_access_key': 'my-aws-secret-access-key-id',
+            'key_name': 'my-ssh-key',
+            'key_filename': '/home/me/.ssh/id_rsa',
+            'status': 'running',
+            'name': 'ec2-resource-1'
+        },
+        'ec2-resource-2': {
+            'id': 'i-3333f40de2b9b8967',
+            'type': 'aws-ec2',
+            'access_key_id': 'my-aws-access-key-id',
+            'secret_access_key': 'my-aws-secret-access-key-id',
+            'key_name': 'my-ssh-key',
+            'key_filename': '/home/me/.ssh/id_rsa',
+            'status': 'stopped',
+            'name': 'ec2-resource-2'
+        }
+    }
+    manager.save_inventory = lambda: None  # Don't save test data to file system.
+    return manager
+
+
 def test_ls_interface():
     """
     Test listing the resources.
     """
 
-    with patch('docker.Client') as docker_client, \
-        patch('boto3.resource') as aws_client, \
-        patch('reproman.resource.ResourceManager._get_inventory') as get_inventory, \
-        swallow_logs(new_level=logging.DEBUG) as log:
-
-        docker_client.return_value = MagicMock(
-            containers=lambda all: [
-                {
-                    'Id': '326b0fdfbf83',
-                    'Names': ['/my-resource'],
-                    'State': 'running'
-                }
-            ]
-        )
+    with patch('docker.Client'), \
+            patch('reproman.interface.ls.get_manager', new=mock_get_manager), \
+            swallow_logs(new_level=logging.DEBUG) as log:
 
-        aws_client.side_effect = [
-            MagicMock(Instance=lambda id: MagicMock(
-                instance_id='i-22221ddf096c22bb0',
-                state={'Name': 'running'}
-            )),
-            MagicMock(Instance=lambda id: MagicMock(
-                instance_id='i-3333f40de2b9b8967',
-                state={'Name': 'stopped'}
-            )),
-        ]
-
-        get_inventory.return_value = {
-            "docker-resource-1": {
-                "status": "running",
-                "engine_url": "tcp://127.0.0.1:2375",
-                "type": "docker-container",
-                "name": "my-resource",
-                "id": "326b0fdfbf838"
-            },
-            "ec2-resource-1": {
-                'id': 'i-22221ddf096c22bb0',
-                'type': 'aws-ec2',
-                'access_key_id': 'my-aws-access-key-id',
-                'secret_access_key': 'my-aws-secret-access-key-id',
-                'key_name': 'my-ssh-key',
-                'key_filename': '/home/me/.ssh/id_rsa',
-                "status": "running",
-                "name": "aws-resource-1"
-            },
-            "ec2-resource-2": {
-                'id': 'i-3333f40de2b9b8967',
-                'type': 'aws-ec2',
-                'access_key_id': 'my-aws-access-key-id',
-                'secret_access_key': 'my-aws-secret-access-key-id',
-                'key_name': 'my-ssh-key',
-                'key_filename': '/home/me/.ssh/id_rsa',
-                "status": "stopped",
-                "name": "aws-resource-2"
-            }
-        }
-
-        args = [
-            'ls',
-        ]
-        with patch("reproman.interface.login.get_manager",
-                   return_value=ResourceManager()):
-            main(args)
-
-        assert_in(
-            'list result: docker-resource-1, docker-container, 326b0fdfbf838, running',
-            log.lines)
+        main(['ls'])
+        assert_in('list result: docker-resource-1, docker-container, 326b0fdfbf838, running', log.lines)
         assert_in('list result: ec2-resource-1, aws-ec2, i-22221ddf096c22bb0, running', log.lines)
-        assert_in('list result: ec2-resource-2, aws-ec2, i-3333f40de2b9b8967, stopped', log.lines)
\ No newline at end of file
+        assert_in('list result: ec2-resource-2, aws-ec2, i-3333f40de2b9b8967, stopped', log.lines)
+
+        # Test --refresh output
+        main(['ls', '--refresh'])
+        assert_in('list result: docker-resource-1, docker-container, 326b0fdfbf838, NOT FOUND', log.lines)
+        assert_in('list result: ec2-resource-1, aws-ec2, i-22221ddf096c22bb0, CONNECTION ERROR', log.lines)
+        assert_in('list result: ec2-resource-2, aws-ec2, i-3333f40de2b9b8967, CONNECTION ERROR', log.lines)
diff --git a/reproman/resource/tests/test_base.py b/reproman/resource/tests/test_base.py
index 5684715..276d2e2 100644
--- a/reproman/resource/tests/test_base.py
+++ b/reproman/resource/tests/test_base.py
@@ -85,7 +85,7 @@ def test_resource_manager_save(tmpdir):
         "null-id": {"name": "null-id",
                     "id": None,
                     "type": "baz-type"}}
-    manager._save()
+    manager.save_inventory()
     assert op.exists(inventory)
     with open(inventory) as fh:
         content = fh.read()
@@ -99,7 +99,7 @@ def test_resource_manager_save(tmpdir):
     manager_reborn.inventory["added"] = {"name": "added",
                                          "type": "added-type",
                                          "id": "added-id"}
-    manager_reborn._save()
+    manager_reborn.save_inventory()
     with open(inventory) as fh:
         content_reread = fh.read()
     for line in content: