Repository: kangasta/pullnrun

Issue:
Add optional errors field to responses

Discussion:

Patch:
diff --git a/.gitignore b/.gitignore
index c947022..765446c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,6 +3,7 @@ __pycache__/
 
 # Test coverage
 .coverage
+htmlcov/
 
 # Build results
 build/
diff --git a/examples/fizzbuzz.json b/examples/fizzbuzz.json
index 511ee9e..48cb10f 100644
--- a/examples/fizzbuzz.json
+++ b/examples/fizzbuzz.json
@@ -1,5 +1,5 @@
 {
-	"$schema": "../pullnrun/schema.json",
+	"$schema": "../pullnrun/schemas/input.json",
 	"pull": {
 		"from": "url",
 		"url": "https://gist.githubusercontent.com/kangasta/7a923e37a544df779552c95ac67f55e8/raw/525d82f42a0fe89f1fb378cb61d8d27d2a006372/fizzbuzz.py"
diff --git a/examples/timetbl.json b/examples/timetbl.json
index 61cf79c..2a7ba96 100644
--- a/examples/timetbl.json
+++ b/examples/timetbl.json
@@ -1,1 +1,1 @@
-{"$schema":"../pullnrun/schema.json","pull":{"from":"url","url":"https://raw.githubusercontent.com/kangasta/timetbl/master/tst/check_online_status.robot"},"run":{"command":["robot","."]}}
\ No newline at end of file
+{"$schema":"../pullnrun/schemas/input.json","pull":{"from":"url","url":"https://raw.githubusercontent.com/kangasta/timetbl/master/tst/check_online_status.robot"},"run":{"command":["robot","."]}}
\ No newline at end of file
diff --git a/pullnrun/_main.py b/pullnrun/_main.py
index 343333b..6c35f3e 100644
--- a/pullnrun/_main.py
+++ b/pullnrun/_main.py
@@ -1,13 +1,4 @@
-import json
-
-try:
-    import importlib.resources as resources
-except ImportError: # pragma: no cover
-    import importlib_resources as resources
-
-from jsonschema import validate
-
-from ._utils import as_list
+from ._utils import as_list, validate_dict
 from ._log import Log
 
 from ._pull import pull
@@ -20,12 +11,8 @@ FUNCTION_MAPPINGS = {
     'push': push,
 }
 
-def _validate(input_dict):
-    schema = json.loads(resources.read_text('pullnrun', 'schema.json'))
-    validate(instance=input_dict, schema=schema)
-
 def main(input_dict, quiet=False):
-    _validate(input_dict)
+    validate_dict(input_dict, 'input')
 
     log_targets = input_dict.get('log')
     log = Log(quiet, log_targets)
diff --git a/pullnrun/_pull.py b/pullnrun/_pull.py
index ae2ead2..33c45a7 100644
--- a/pullnrun/_pull.py
+++ b/pullnrun/_pull.py
@@ -22,6 +22,7 @@ def _pull_http(url, headers=None, filename=None, extract=False, log=void_fn):
 
     start = timestamp()
     log(get_log_entry('pull_http', status, url=url, filename=filename, start=start))
+    errors = []
 
     try:
         with get(url, headers=headers, stream=True) as r:
@@ -33,12 +34,13 @@ def _pull_http(url, headers=None, filename=None, extract=False, log=void_fn):
         if extract:
             unpack_archive(filename)
         status = 'SUCCESS'
-    except:
+    except Exception as e:
         status = 'ERROR'
+        errors.append(f'Downloading or unpacking the file failed. ({type(e).__name__})')
 
     end = timestamp()
 
-    log(get_log_entry('pull_http', status, url=url, filename=filename, status_code=status_code, extracted=extract, start=start, end=end))
+    log(get_log_entry('pull_http', status, url=url, filename=filename, status_code=status_code, extracted=extract, start=start, end=end, errors=errors))
     return status == 'SUCCESS'
 
 def _pull_s3(bucket, object_name, filename=None, log=void_fn):
@@ -47,22 +49,23 @@ def _pull_s3(bucket, object_name, filename=None, log=void_fn):
 
     status = 'STARTED'
 
-    try:
-        s3 = boto3.client('s3')
-    except NameError:
-        pass
-
     start = timestamp()
     log(get_log_entry('pull_s3', status, bucket=bucket, object_name=object_name, filename=filename, start=start))
+    errors = []
 
     try:
+        s3 = boto3.client('s3')
         s3.download_file(bucket, object_name, filename)
         status = 'SUCCESS'
-    except:
+    except NameError:
+        status = 'ERROR'
+        errors.append('boto3 library not found. (NameError)')
+    except Exception as e:
         status = 'ERROR'
+        errors.append(f'Downloading the file from S3 failed. ({type(e).__name__})')
     end = timestamp()
 
-    log(get_log_entry('pull_s3', status, bucket=bucket, object_name=object_name, filename=filename, start=start, end=end))
+    log(get_log_entry('pull_s3', status, bucket=bucket, object_name=object_name, filename=filename, start=start, end=end, errors=errors))
     return status == 'SUCCESS'
 
 def pull(log, **kwargs):
diff --git a/pullnrun/_push.py b/pullnrun/_push.py
index 2dac210..a1878cc 100644
--- a/pullnrun/_push.py
+++ b/pullnrun/_push.py
@@ -13,6 +13,7 @@ def _push_http(filename, url, method='PUT', data=None, headers=None, log=void_fn
 
     start = timestamp()
     log(get_log_entry('push_http', status, start=start, url=url, filename=filename))
+    errors = []
 
     try:
         with open(filename, 'rb') as f:
@@ -21,11 +22,12 @@ def _push_http(filename, url, method='PUT', data=None, headers=None, log=void_fn
             status_code = r.status_code
             r.raise_for_status()
             status = 'SUCCESS'
-    except:
+    except Exception as e:
+        errors.append(f'Uploading the file failed. ({type(e).__name__})')
         status = 'ERROR'
     end = timestamp()
 
-    log(get_log_entry('push_http', status, start=start, end=end, url=url, filename=filename, status_code=status_code))
+    log(get_log_entry('push_http', status, start=start, end=end, url=url, filename=filename, status_code=status_code, errors=errors))
     return status == 'SUCCESS'
 
 def _push_s3(bucket, filename, object_name=None, prefix=None, log=void_fn):
@@ -36,22 +38,23 @@ def _push_s3(bucket, filename, object_name=None, prefix=None, log=void_fn):
 
     status = 'STARTED'
 
-    try:
-        s3 = boto3.client('s3')
-    except NameError:
-        pass
-
     start = timestamp()
     log(get_log_entry('push_s3', status, start=start, filename=filename, bucket=bucket, object_name=object_name))
+    errors = []
 
     try:
+        s3 = boto3.client('s3')
         s3.upload_file(filename, bucket, object_name)
         status = 'SUCCESS'
-    except:
+    except NameError:
+        status = 'ERROR'
+        errors.append('boto3 library not found. (NameError)')
+    except Exception as e:
+        errors.append(f'Uploading the file failed. ({type(e).__name__})')
         status = 'ERROR'
     end = timestamp()
 
-    log(get_log_entry('push_s3', status, start=start, end=end, filename=filename, bucket=bucket, object_name=object_name))
+    log(get_log_entry('push_s3', status, start=start, end=end, filename=filename, bucket=bucket, object_name=object_name, errors=errors))
     return status == 'SUCCESS'
 
 
diff --git a/pullnrun/_run.py b/pullnrun/_run.py
index 7a79b54..f2bfecb 100644
--- a/pullnrun/_run.py
+++ b/pullnrun/_run.py
@@ -12,6 +12,7 @@ def run(log, command, directory=None, **_):
     status = 'STARTED'
     returncode = None
     stdout = None
+    errors = []
 
     start = timestamp()
     log(get_log_entry('run', status, command=command, start=start))
@@ -21,9 +22,10 @@ def run(log, command, directory=None, **_):
         returncode = cp.returncode
         status = 'SUCCESS' if returncode == 0 else 'FAIL'
         stdout = str(cp.stdout)
-    except:
+    except Exception as e:
+        errors.append(f'Executing the command failed. ({type(e).__name__})')
         status = 'ERROR'
     end = timestamp()
 
-    log(get_log_entry('run', status, command=command, exit_code=returncode, output=stdout, start=start, end=end))
+    log(get_log_entry('run', status, command=command, exit_code=returncode, output=stdout, start=start, end=end, errors=errors))
     return status == 'SUCCESS'
diff --git a/pullnrun/_utils.py b/pullnrun/_utils.py
index bcb1bb6..9857606 100644
--- a/pullnrun/_utils.py
+++ b/pullnrun/_utils.py
@@ -1,6 +1,13 @@
 import json
 from time import time
 
+try:
+    import importlib.resources as resources
+except ImportError: # pragma: no cover
+    import importlib_resources as resources
+
+from jsonschema import validate
+
 def as_list(a):
     if isinstance(a, list):
         return a
@@ -23,14 +30,25 @@ def create_meta(start, end, **kwargs):
 def filter_dict(dict_, keys):
     return {k: v for k, v in dict_.items() if k in keys}
 
-def get_log_entry(type_, status, start=None, end=None, **data):
-    return {
+def get_log_entry(type_, status, start=None, end=None, errors=None, **data):
+    try:
+        json.dumps(data)
+    except:
+        raise ValueError('data should be JSON serializable')
+
+    entry = {
         'type': type_,
         'status': status,
         'data': data,
         'meta': create_meta(start, end)
     }
 
+    if errors:
+        entry['errors'] = errors
+
+    validate_dict(entry, 'log_entry')
+    return entry
+
 def prefix_object(prefix, object_name, delimiter='/'):
     name_as_list = object_name.split(delimiter)
     folder = delimiter.join(name_as_list[:-1])
@@ -39,5 +57,9 @@ def prefix_object(prefix, object_name, delimiter='/'):
 def timestamp():
     return int(time() * 1000)
 
+def validate_dict(input_dict, schema_name):
+    schema = json.loads(resources.read_text('pullnrun.schemas', f'{schema_name}.json'))
+    validate(instance=input_dict, schema=schema)
+
 def void_fn(*_args, **_kwargs):
     return None
\ No newline at end of file
diff --git a/pullnrun/schemas/__init__.py b/pullnrun/schemas/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/pullnrun/schema.json b/pullnrun/schemas/input.json
similarity index 100%
rename from pullnrun/schema.json
rename to pullnrun/schemas/input.json
diff --git a/pullnrun/schemas/log_entry.json b/pullnrun/schemas/log_entry.json
new file mode 100644
index 0000000..e5b90d0
--- /dev/null
+++ b/pullnrun/schemas/log_entry.json
@@ -0,0 +1,31 @@
+{
+	"$schema": "http://json-schema.org/draft-07/schema#",
+	"definitions": {
+		"timestamp": {
+			"oneOf": [
+				{ "type": "number", "$comment": "timestamp in ms" },
+				{ "type": "null" }
+			]
+		},
+		"meta": {
+			"type": "object",
+			"properties": {
+				"start": { "$ref": "#/definitions/timestamp"},
+				"end": { "$ref": "#/definitions/timestamp"}
+			},
+			"required": ["start", "end"]
+		}
+	},
+	"type": "object",
+	"properties": {
+		"type": { "type": "string" },
+		"status": { "type": "string" },
+		"data": { "type": "object" },
+		"meta": { "$ref": "#/definitions/meta" },
+		"errors": {
+			"type": "array",
+			"items": { "type": "string" }
+		}
+	},
+	"required": ["type", "status", "data", "meta"]
+}
\ No newline at end of file
diff --git a/setup.py b/setup.py
index d84af72..dcc00b2 100644
--- a/setup.py
+++ b/setup.py
@@ -15,7 +15,7 @@ setuptools.setup(
 	url="https://github.com/kangasta/pullnrun",
 	packages=setuptools.find_packages(),
 	package_data={
-		'pullnrun': ['schema.json']
+		'pullnrun': ['schemas/*.json']
 	},
 	scripts=["bin/pullnrun"],
 	install_requires=[


Test patch:
diff --git a/tst/test_pull.py b/tst/test_pull.py
index 3c4d54e..a6078d0 100644
--- a/tst/test_pull.py
+++ b/tst/test_pull.py
@@ -1,5 +1,5 @@
 from unittest import TestCase
-from unittest.mock import patch, Mock
+from unittest.mock import patch, MagicMock
 
 from pullnrun._utils import void_fn
 from pullnrun._pull import pull
@@ -13,11 +13,13 @@ PULL_S3 = {
 class PullTest(TestCase):
     @patch('boto3.client', side_effect=NameError)
     def test_pull_s3_handles_missing_boto3(self, mock):
-        self.assertFalse(pull(void_fn, **PULL_S3))
+        log_fn = MagicMock()
+        self.assertFalse(pull(log_fn, **PULL_S3))
+        self.assertIn('boto3 library not found', log_fn.call_args[-2][0].get('errors')[0]) # pylint: disable=unsubscriptable-object
 
     @patch('boto3.client')
     def test_pull_s3_calls_download_file(self, mock):
-        s3_mock = Mock()
+        s3_mock = MagicMock()
         mock.return_value = s3_mock
 
         r = pull(void_fn, **PULL_S3)
@@ -27,8 +29,8 @@ class PullTest(TestCase):
 
     @patch('boto3.client')
     def test_pull_s3_handles_failing_download(self, mock):
-        s3_mock = Mock()
-        s3_mock.download_file.side_effect = KeyboardInterrupt
+        s3_mock = MagicMock()
+        s3_mock.download_file.side_effect = Exception
         mock.return_value = s3_mock
 
         r = pull(void_fn, **PULL_S3)
diff --git a/tst/test_push.py b/tst/test_push.py
index a4b500e..a81edab 100644
--- a/tst/test_push.py
+++ b/tst/test_push.py
@@ -1,5 +1,5 @@
 from unittest import TestCase
-from unittest.mock import patch, Mock
+from unittest.mock import patch, MagicMock
 
 from pullnrun._utils import void_fn
 from pullnrun._push import push
@@ -14,11 +14,13 @@ PUSH_S3 = {
 class PushTest(TestCase):
     @patch('boto3.client', side_effect=NameError)
     def test_push_s3_handles_missing_boto3(self, mock):
-        self.assertFalse(push(void_fn, **PUSH_S3))
+        log_fn = MagicMock()
+        self.assertFalse(push(log_fn, **PUSH_S3))
+        self.assertIn('boto3 library not found', log_fn.call_args[-2][0].get('errors')[0]) # pylint: disable=unsubscriptable-object
 
     @patch('boto3.client')
     def test_push_s3_calls_upload_file(self, mock):
-        s3_mock = Mock()
+        s3_mock = MagicMock()
         mock.return_value = s3_mock
 
         r = push(void_fn, **PUSH_S3)
@@ -28,8 +30,8 @@ class PushTest(TestCase):
 
     @patch('boto3.client')
     def test_push_s3_handles_failing_upload(self, mock):
-        s3_mock = Mock()
-        s3_mock.upload_file.side_effect = KeyboardInterrupt
+        s3_mock = MagicMock()
+        s3_mock.upload_file.side_effect = Exception
         mock.return_value = s3_mock
 
         r = push(void_fn, **PUSH_S3)
@@ -39,7 +41,7 @@ class PushTest(TestCase):
 
     @patch('boto3.client')
     def test_prefixes_name_with_id_by_default(self, mock):
-        s3_mock = Mock()
+        s3_mock = MagicMock()
         mock.return_value = s3_mock
 
         push_s3 = {**PUSH_S3}
diff --git a/tst/test_run.py b/tst/test_run.py
index 024f113..aaf42e7 100644
--- a/tst/test_run.py
+++ b/tst/test_run.py
@@ -27,7 +27,7 @@ class RunTest(TestCase):
 
         self.assertEqual(log.last['status'], 'FAIL')
 
-    @patch('subprocess.run', side_effect=KeyboardInterrupt)
+    @patch('subprocess.run', side_effect=Exception)
     def test_run_handles_failing_subprocess_run(self, mock_run):
         log = LogMock()
         self.assertFalse(run(log, ['cmd']))
diff --git a/tst/test_utils.py b/tst/test_utils.py
index 9f21e01..feafa23 100644
--- a/tst/test_utils.py
+++ b/tst/test_utils.py
@@ -1,7 +1,9 @@
 from unittest import TestCase
 from time import time
 
-from pullnrun._utils import timestamp, as_list, create_meta, filter_dict, void_fn, prefix_object
+from jsonschema.exceptions import ValidationError
+
+from pullnrun._utils import timestamp, as_list, create_meta, filter_dict, get_log_entry, void_fn, prefix_object
 
 class UtilsTest(TestCase):
     def test_as_list_ensures_variable_is_list(self):
@@ -54,4 +56,22 @@ class UtilsTest(TestCase):
         self.assertEqual(prefix_object('a','b'), 'a-b')
         self.assertEqual(prefix_object('b', 'a/c'), 'a/b-c')
         self.assertEqual(prefix_object('b', 'a/c', '_'), 'b-a/c')
-        self.assertEqual(prefix_object('b', 'a_c', '_'), 'a_b-c')
\ No newline at end of file
+        self.assertEqual(prefix_object('b', 'a_c', '_'), 'a_b-c')
+
+    def test_get_log_entry_validates_output(self):
+        data = [
+            (1, 'STARTED'),
+            ('run', None),
+            ('run', 'STARTED', "asd"),
+            ('run', 'STARTED', 123, None, [{'asd': 123}]),
+        ]
+
+        for i in data:
+            with self.assertRaises(ValidationError):
+                get_log_entry(*i)
+
+    def test_get_log_entry_check_data(self):
+        args = ('run', 'STARTED',)
+
+        with self.assertRaises(ValueError):
+            get_log_entry(*args, invalid=(i for i in range(5)))