From 0d092d042de12c845f3e0dacead99e52dd8de74e Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Mon, 1 Apr 2019 10:37:45 -0400 Subject: [PATCH 1/4] setup.py: Add pyout --- setup.py | 1 + 1 file changed, 1 insertion(+) diff --git a/setup.py b/setup.py index eab94f333..3b9d50c8c 100755 --- a/setup.py +++ b/setup.py @@ -54,6 +54,7 @@ def findsome(subdir, extensions): 'scp', 'pycrypto', 'pyOpenSSL==16.2.0', + 'pyout>=0.4.0', 'requests', 'reprozip; sys_platform=="linux" or sys_platform=="linux2"', 'rpaths', From 1cfe185256fcb8419106ee8db250e3d89379c58d Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Mon, 1 Apr 2019 10:29:10 -0400 Subject: [PATCH 2/4] TST: ls: Adjust return structure to mirror pyout Ls.__call__() returns a dictionary with results to ease testing. ls.py will start using pyout in the next commit. Once that happens, it makes sense to drop the custom result dictionary because the Tabular object already contains the same information and provides access to the values _after_ the asynchronous functions. But before we do that, convert the result dictionary to a structure that more closely matches the Tabular getitem interface so that we can switch to pyout *without* modifying the actual tests. --- reproman/interface/ls.py | 3 ++- reproman/interface/tests/test_ls.py | 37 +++++++++++++++++------------ 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/reproman/interface/ls.py b/reproman/interface/ls.py index fff0fdd8c..27758df60 100644 --- a/reproman/interface/ls.py +++ b/reproman/interface/ls.py @@ -94,7 +94,8 @@ def __call__(resrefs=None, verbose=False, refresh=False): resource.status, ) ui.message(template.format(*msgargs)) - results[id_] = msgargs + results[(name,)] = dict(zip(["name", "type", "id", "status"], + msgargs)) if refresh: manager.save_inventory() diff --git a/reproman/interface/tests/test_ls.py b/reproman/interface/tests/test_ls.py index ea88900ac..b1868898a 100644 --- a/reproman/interface/tests/test_ls.py +++ b/reproman/interface/tests/test_ls.py @@ -7,6 +7,7 @@ # ## ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ## import contextlib +from io import StringIO from unittest.mock import patch import pytest @@ -54,13 +55,17 @@ def resource_manager(): @pytest.fixture(scope="function") def ls_fn(resource_manager): + stream = StringIO() + def fn(*args, **kwargs): skipif.no_docker_dependencies() with contextlib.ExitStack() as stack: stack.enter_context(patch("docker.Client")) stack.enter_context(patch("reproman.interface.ls.get_manager", return_value=resource_manager)) - return ls(*args, **kwargs) + stack.enter_context(patch("reproman.interface.ls.ui._ui.out", + stream)) + return ls(*args, **kwargs), stream.getvalue() return fn @@ -68,22 +73,24 @@ def test_ls_interface(ls_fn): """ Test listing the resources. """ - results = ls_fn() - assert "running" in results["326b0fdfbf838"] - assert "docker-container" in results["326b0fdfbf838"] - assert "i-22221ddf096c22bb0" in results - assert "stopped" in results["i-3333f40de2b9b8967"] - assert "aws-ec2" in results["i-3333f40de2b9b8967"] + table, _ = ls_fn() + dr1 = table[("docker-resource-1",)] + assert dr1["status"] == "running" + assert dr1["type"] == "docker-container" + assert table[("ec2-resource-1",)]["id"] == "i-22221ddf096c22bb0" + er2 = table[("ec2-resource-2",)] + assert er2["status"] == "stopped" + assert er2["type"] == "aws-ec2" # Test --refresh output - results = ls_fn(refresh=True) - assert "NOT FOUND" in results["326b0fdfbf838"] - assert "CONNECTION ERROR" in results["i-22221ddf096c22bb0"] - assert "CONNECTION ERROR" in results["i-3333f40de2b9b8967"] + table, _ = ls_fn(refresh=True) + assert table[("docker-resource-1",)]["status"] == "NOT FOUND" + assert table[("ec2-resource-1",)]["status"] == "CONNECTION ERROR" + assert table[("ec2-resource-2",)]["status"] == "CONNECTION ERROR" def test_ls_interface_limited(ls_fn): - results = ls_fn(resrefs=["326", "i-33"]) - assert "326b0fdfbf838" in results - assert "i-22221ddf096c22bb0" not in results - assert "i-3333f40de2b9b8967" in results + _, stdout = ls_fn(resrefs=["326", "i-33"]) + assert "326b0fdfbf838" in stdout + assert "i-22221ddf096c22bb0" not in stdout + assert "i-3333f40de2b9b8967" in stdout From 8ed7a7a89522665e52412c2f74248d7636c76a56 Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Mon, 1 Apr 2019 12:32:56 -0400 Subject: [PATCH 3/4] TST: ls: Check handling of non-existent resource --- reproman/interface/tests/test_ls.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/reproman/interface/tests/test_ls.py b/reproman/interface/tests/test_ls.py index b1868898a..21cdcd1e5 100644 --- a/reproman/interface/tests/test_ls.py +++ b/reproman/interface/tests/test_ls.py @@ -8,11 +8,13 @@ import contextlib from io import StringIO +import logging from unittest.mock import patch import pytest from ...api import ls +from ...utils import swallow_logs from ...resource.base import ResourceManager from ...tests.skip import skipif @@ -94,3 +96,10 @@ def test_ls_interface_limited(ls_fn): assert "326b0fdfbf838" in stdout assert "i-22221ddf096c22bb0" not in stdout assert "i-3333f40de2b9b8967" in stdout + + +def test_ls_interface_missing(ls_fn): + with swallow_logs(new_level=logging.WARNING) as log: + _, stdout = ls_fn(resrefs=["idonotexist"]) + assert "idonotexist" not in stdout + assert "idonotexist" in log.out From afd47efccf80c406fed525e6521268bfed9bfcd4 Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Thu, 21 Mar 2019 10:52:06 -0400 Subject: [PATCH 4/4] ENH: ls: Use pyout to render table The main benefit of using pyout here is that we can query statuses asynchronously, but it also allows us to style the output in more complex ways (automatic truncation, coloring by regexp, ...) with less custom code than we'd need otherwise. This switch to pyout should not change how we interpret or update the resource statuses, but the code for interacting with the resources is a little more complicated because, when --refresh is given, we access the resources in functions that are called asynchronously. And to update the resource statuses, we need to look at the values after the asynchronous calls have returned. Closes #318. --- reproman/interface/ls.py | 108 ++++++++++++++++++---------- reproman/interface/tests/test_ls.py | 7 +- 2 files changed, 77 insertions(+), 38 deletions(-) diff --git a/reproman/interface/ls.py b/reproman/interface/ls.py index 27758df60..974cf3fe8 100644 --- a/reproman/interface/ls.py +++ b/reproman/interface/ls.py @@ -11,7 +11,7 @@ __docformat__ = 'restructuredtext' -from collections import OrderedDict +from functools import partial from .base import Interface # import reproman.interface.base # Needed for test patching @@ -55,50 +55,86 @@ class Ls(Interface): @staticmethod def __call__(resrefs=None, verbose=False, refresh=False): - id_length = 19 # todo: make it possible to output them long - template = '{:<20} {:<20} {:<%(id_length)s} {!s:<10}' % locals() - ui.message(template.format('RESOURCE NAME', 'TYPE', 'ID', 'STATUS')) - ui.message(template.format('-------------', '----', '--', '------')) + from pyout import Tabular - results = OrderedDict() manager = get_manager() if not resrefs: resrefs = (manager.inventory[n]["id"] for n in sorted(manager) if not n.startswith("_")) - for resref in resrefs: - try: - resource = manager.get_resource(resref) - name = resource.name - except ResourceError as e: - lgr.warning("Manager did not return a resource for %s: %s", - resref, exc_str(e)) - continue - + table = Tabular( + # Note: We're going with the name as the row key even though ID + # would be the more natural choice because (1) inventory already + # uses the name as the key, so we know it's unique and (2) sadly we + # can't rely on the ID saying set after a .connect() calls (e.g., + # see docker_container.connect()). + ["name", "type", "id", "status"], + style={ + "default_": {"width": {"marker": "…", "truncate": "center"}}, + "header_": {"underline": True, + "transform": str.upper}, + "status": {"color": + {"re_lookup": [["^running$", "green"], + ["^(stopped|exited)$", "red"], + ["(ERROR|NOT FOUND)", "red"]]}, + "bold": + {"re_lookup": [["(ERROR|NOT FOUND)", True]]}}}) + + def get_status(res): if refresh: + def fn(): + try: + res.connect() + except Exception as e: + status = 'CONNECTION ERROR' + else: + status = res.status if res.id else 'NOT FOUND' + return status + return "querying…", fn + else: + return res.status + + # Store a list of actions to do after the table is finalized so that we + # don't interrupt the table's output. + do_after = [] + # The refresh happens in an asynchronous call. Keep a list of resources + # that we should ask pyout about once the table is finalized. + resources_to_refresh = [] + with table: + for resref in resrefs: try: - resource.connect() - 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}) - - id_ = manager.inventory[name]['id'] - msgargs = ( - name, - resource.type, - id_[:id_length], - resource.status, - ) - ui.message(template.format(*msgargs)) - results[(name,)] = dict(zip(["name", "type", "id", "status"], - msgargs)) + resource = manager.get_resource(resref) + name = resource.name + except ResourceError as e: + do_after.append( + partial(lgr.warning, + "Manager did not return a resource for %s: %s", + resref, + exc_str(e))) + continue + + id_ = manager.inventory[name]['id'] + assert id_ == resource.id, "bug in resource logic" + table([name, + resource.type, + id_, + get_status(resource)]) + resources_to_refresh.append(resource) + + if do_after or not refresh: + # Distinguish between the table and added information. + ui.message("\n") + + for fn in do_after: + fn() if refresh: - manager.save_inventory() + if resources_to_refresh: + for res in resources_to_refresh: + name = res.name + status = table[(name,)]["status"] + manager.inventory[name].update({'status': status}) + manager.save_inventory() else: ui.message('Use --refresh option to view updated status.') - return results + return table diff --git a/reproman/interface/tests/test_ls.py b/reproman/interface/tests/test_ls.py index 21cdcd1e5..2468e0ac8 100644 --- a/reproman/interface/tests/test_ls.py +++ b/reproman/interface/tests/test_ls.py @@ -7,12 +7,15 @@ # ## ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ## import contextlib +from functools import partial from io import StringIO import logging from unittest.mock import patch import pytest +from pyout import Tabular + from ...api import ls from ...utils import swallow_logs from ...resource.base import ResourceManager @@ -58,6 +61,7 @@ def resource_manager(): @pytest.fixture(scope="function") def ls_fn(resource_manager): stream = StringIO() + TestTabular = partial(Tabular, stream=stream) def fn(*args, **kwargs): skipif.no_docker_dependencies() @@ -65,8 +69,7 @@ def fn(*args, **kwargs): stack.enter_context(patch("docker.Client")) stack.enter_context(patch("reproman.interface.ls.get_manager", return_value=resource_manager)) - stack.enter_context(patch("reproman.interface.ls.ui._ui.out", - stream)) + stack.enter_context(patch("pyout.Tabular", TestTabular)) return ls(*args, **kwargs), stream.getvalue() return fn