Merge pull request #66 from chmeliik/error-reporting

Error reporting

Author: kevinrizza

operatorcourier/api.py

@@ -15,6 +15,7 @@
 from operatorcourier.push import PushCmd
 from operatorcourier.format import format_bundle
 from operatorcourier.nest import nest_bundles
+from operatorcourier.errors import OpCourierBadBundle
 
 logger = logging.getLogger(__name__)
 
@@ -31,6 +32,12 @@ def build_and_verify(source_dir=None, yamls=None, ui_validate_io=False,
     :param ui_validate_io: Optional flag to test operatorhub.io specific validation
     :param validation_output: Path to optional output file for validation logs
     :param repository: Repository name for the application
+
+    :raises TypeError: When called with both source_dir and yamls specified
+
+    :raises OpCourierBadYaml: When an invalid yaml file is encountered
+    :raises OpCourierBadArtifact: When a file is not any of {CSV, CRD, Package}
+    :raises OpCourierBadBundle: When the resulting bundle fails validation
     """
 
     if source_dir is not None and yamls is not None:
@@ -53,17 +60,19 @@ def build_and_verify(source_dir=None, yamls=None, ui_validate_io=False,
     valid, validation_results_dict = ValidateCmd(ui_validate_io).validate(bundle,
                                                                           repository)
 
-    if not valid:
-        bundle = None
-        logger.error("Bundle failed validation.")
-        raise ValueError("Resulting bundle is invalid, input yaml is improperly defined.")
-    else:
-        bundle = format_bundle(bundle)
-
     if validation_output is not None:
         with open(validation_output, 'w') as f:
             f.write(json.dumps(validation_results_dict) + "\n")
-    return bundle
+
+    if valid:
+        bundle = format_bundle(bundle)
+        return bundle
+    else:
+        logger.error("Bundle failed validation.")
+        raise OpCourierBadBundle(
+            "Resulting bundle is invalid, input yaml is improperly defined.",
+            validation_info=validation_results_dict
+        )
 
 
 def build_verify_and_push(namespace, repository, revision, token,
@@ -82,6 +91,16 @@ def build_verify_and_push(namespace, repository, revision, token,
     :param source_dir: Path to local directory of yaml files to be read
     :param yamls: List of yaml strings to create bundle with
     :param validation_output: Path to optional output file for validation logs
+
+    :raises TypeError: When called with both source_dir and yamls specified
+
+    :raises OpCourierBadYaml: When an invalid yaml file is encountered
+    :raises OpCourierBadArtifact: When a file is not any of {CSV, CRD, Package}
+    :raises OpCourierBadBundle: When the resulting bundle fails validation
+
+    :raises OpCourierQuayCommunicationError: When communication with Quay fails
+    :raises OpCourierQuayErrorResponse: When Quay responds with an error
+    :raises OpCourierQuayError: When the request fails in an unexpected way
     """
 
     bundle = build_and_verify(source_dir, yamls, repository=repository,
@@ -102,6 +121,9 @@ def nest(source_dir, registry_dir):
     :param source_dir: Path to local directory of yaml files to be read
     :param output_dir: Path of your directory to be populated.
                        If directory does not exist, it will be created.
+
+    :raises OpCourierBadYaml: When an invalid yaml file is encountered
+    :raises OpCourierBadArtifact: When a file is not any of {CSV, CRD, Package}
     """
 
     yaml_files = []

operatorcourier/errors.py

@@ -0,0 +1,64 @@
+"""
+operatorcourier.errors
+
+Defines custom operator-courier exceptions.
+"""
+from requests import RequestException
+
+
+class OpCourierError(Exception):
+    """Base class for all operator-courier exceptions"""
+    pass
+
+
+class OpCourierValueError(OpCourierError, ValueError):
+    """Base class for exceptions that need to inherit from ValueError
+    for backwards compatibility
+    """
+    pass
+
+
+class OpCourierBadYaml(OpCourierValueError):
+    """Invalid yaml file"""
+    pass
+
+
+class OpCourierBadArtifact(OpCourierValueError):
+    """File is a valid yaml, but not a valid CSV, CRD or Package"""
+    pass
+
+
+class OpCourierBadBundle(OpCourierValueError):
+    """Invalid bundle (e.g. missing CSV/CRD/Package).
+    Contains the info collected during validation of the bundle.
+    """
+    def __init__(self, msg, validation_info):
+        """
+        :param msg: the message of this exception
+        :param validation_info: info collected during validation of bundle
+        """
+        super().__init__(msg)
+        self.validation_info = validation_info
+
+
+class OpCourierQuayError(OpCourierError):
+    """An error occurred while pushing bundle to Quay.io"""
+    pass
+
+
+class OpCourierQuayCommunicationError(OpCourierQuayError, RequestException):
+    """Communication with Quay.io failed"""
+    pass
+
+
+class OpCourierQuayErrorResponse(OpCourierQuayError, OpCourierValueError):
+    """Quay.io responded with an error"""
+    def __init__(self, msg, code, error_response):
+        """
+        :param msg: the message of this exception
+        :param code: http status code returned by response
+        :param error_response: error json taken from response
+        """
+        super().__init__(msg)
+        self.code = code
+        self.error_response = error_response

operatorcourier/identify.py

@@ -1,6 +1,7 @@
 from yaml import safe_load
 from yaml import MarkedYAMLError
 import logging
+from operatorcourier.errors import OpCourierBadYaml, OpCourierBadArtifact
 
 logger = logging.getLogger(__name__)
 
@@ -17,7 +18,7 @@ def get_operator_artifact_type(operatorArtifactString):
     except MarkedYAMLError:
         msg = "Courier requires valid input YAML files"
         logger.error(msg)
-        raise ValueError(msg)
+        raise OpCourierBadYaml(msg)
     else:
         artifact_type, artifact_name = None, None
         if isinstance(operatorArtifact, dict):
@@ -34,4 +35,4 @@ def get_operator_artifact_type(operatorArtifactString):
 
         msg = 'Courier requires valid CSV, CRD, and Package files'
         logger.error(msg)
-        raise ValueError(msg)
+        raise OpCourierBadArtifact(msg)

operatorcourier/push.py

@@ -3,6 +3,10 @@
 import tarfile
 import logging
 from tempfile import TemporaryDirectory
+from operatorcourier.errors import (
+    OpCourierQuayCommunicationError,
+    OpCourierQuayErrorResponse
+)
 
 logger = logging.getLogger(__name__)
 
@@ -41,16 +45,23 @@ def _push_to_registry(self, namespace, repository, release, bundle, auth_token):
         logger.info('Pushing bundle to %s' % push_uri)
         headers = {'Content-Type': 'application/json', 'Authorization': auth_token}
         json = {'blob': bundle, 'release': release, "media_type": "helm"}
-        r = requests.post(push_uri, json=json, headers=headers)
+
+        try:
+            r = requests.post(push_uri, json=json, headers=headers)
+        except requests.RequestException as e:
+            msg = str(e)
+            logger.error(msg)
+            raise OpCourierQuayCommunicationError(msg)
+
         if r.status_code != 200:
             logger.error(r.text)
 
-            msg = 'Failed to get error details'
             try:
-                error = r.json()['error']
-                msg = "Quay.io answered: '{}' ({})".format(
-                    error['code'], error['message'])
-            except Exception:
-                pass
+                r_json = r.json()
+            except ValueError:
+                r_json = {}
 
-            raise ValueError("Failed to push to app registry: {}".format(msg))
+            msg = r_json.get('error', {}).get(
+                'message', 'Failed to get error details.'
+            )
+            raise OpCourierQuayErrorResponse(msg, r.status_code, r_json)

tests/test_api.py

@@ -2,6 +2,7 @@
 import yaml
 from operatorcourier import api
 from operatorcourier.format import unformat_bundle
+from operatorcourier.errors import OpCourierBadBundle
 
 
 @pytest.mark.parametrize('directory,expected', [
@@ -37,3 +38,29 @@ def test_make_bundle_with_yaml_list(yaml_files, expected):
     with open(expected, "r") as expected_file:
         expected_bundle = yaml.safe_load(expected_file)
         assert unformat_bundle(bundle) == unformat_bundle(expected_bundle)
+
+
+@pytest.mark.parametrize('yaml_files', [
+    ["tests/test_files/bundles/api/bundle1/crd.yml",
+     "tests/test_files/bundles/api/bundle1/csv.yaml"]
+])
+def test_make_bundle_invalid(yaml_files):
+    yamls = []
+    for file in yaml_files:
+        with open(file, "r") as yaml_file:
+            yamls.append(yaml_file.read())
+
+    with pytest.raises(OpCourierBadBundle) as e_info:
+        api.build_and_verify(yamls=yamls)
+
+    e = e_info.value
+    e_msg = "Resulting bundle is invalid, input yaml is improperly defined."
+    e_dict = {
+        "warnings": [
+            "csv metadata.annotations not defined.",
+            "csv spec.icon not defined"
+        ],
+        "errors": ["Bundle does not contain any packages."]
+    }
+    assert str(e) == e_msg
+    assert e.validation_info == e_dict

tests/test_errors.py

@@ -0,0 +1,73 @@
+import pytest
+from requests import RequestException
+
+from operatorcourier.errors import (
+    OpCourierError,
+    OpCourierValueError,
+
+    OpCourierBadYaml,
+    OpCourierBadArtifact,
+    OpCourierBadBundle,
+
+    OpCourierQuayError,
+    OpCourierQuayCommunicationError,
+    OpCourierQuayErrorResponse
+)
+
+
+@pytest.mark.parametrize('op_courier_exception', [
+    OpCourierValueError,
+
+    OpCourierBadYaml,
+    OpCourierBadArtifact,
+    OpCourierBadBundle,
+
+    OpCourierQuayErrorResponse
+])
+def test_value_error_compatibility(op_courier_exception):
+    """Test that exceptions replacing ValueError are backwards compatible."""
+    assert issubclass(op_courier_exception, ValueError)
+
+
+@pytest.mark.parametrize('op_courier_exception', [
+    OpCourierQuayCommunicationError
+])
+def test_request_exception_compatibility(op_courier_exception):
+    """Test that exceptions replacing RequestException are backwards compatible"""
+    assert (
+        issubclass(op_courier_exception, RequestException)
+        and not issubclass(op_courier_exception, ValueError)
+    )
+
+
+@pytest.mark.parametrize('op_courier_exception', [
+    OpCourierValueError,
+
+    OpCourierBadYaml,
+    OpCourierBadArtifact,
+    OpCourierBadBundle,
+
+    OpCourierQuayError,
+    OpCourierQuayCommunicationError,
+    OpCourierQuayErrorResponse
+])
+def test_base_exception_inheritance(op_courier_exception):
+    """Test that all exceptions are subclasses of the base exception"""
+    assert issubclass(op_courier_exception, OpCourierError)
+
+
+@pytest.mark.parametrize('op_courier_exception', [
+    OpCourierQuayCommunicationError,
+    OpCourierQuayErrorResponse
+])
+def test_quay_exception_inheritance(op_courier_exception):
+    """Test that quay exceptions are subclasses of the base quay exception"""
+    assert issubclass(op_courier_exception, OpCourierQuayError)
+
+
+def test_create_quay_error_response():
+    """Test that creation of quay error responses is working as intended"""
+    e = OpCourierQuayErrorResponse('oh no', 500, {'error': 'uh oh'})
+    assert str(e) == 'oh no'
+    assert e.code == 500
+    assert e.error_response == {'error': 'uh oh'}

tests/test_identify.py

@@ -1,5 +1,6 @@
 import pytest
 import operatorcourier.identify as identify
+from operatorcourier.errors import OpCourierBadYaml, OpCourierBadArtifact
 from testfixtures import LogCapture
 
 
@@ -21,7 +22,7 @@ def test_get_operator_artifact_type(fname, expected):
 def test_get_operator_artifact_type_assertions(fname):
     with open(fname) as f:
         yaml = f.read()
-    with pytest.raises(ValueError) as e, LogCapture() as logs:
+    with pytest.raises(OpCourierBadArtifact) as e, LogCapture() as logs:
         identify.get_operator_artifact_type(yaml)
 
     logs.check(('operatorcourier.identify',
@@ -38,7 +39,7 @@ def test_get_operator_artifact_type_with_invalid_yaml(fname):
     with open(fname) as f:
         yaml = f.read()
 
-    with pytest.raises(ValueError) as e, LogCapture() as logs:
+    with pytest.raises(OpCourierBadYaml) as e, LogCapture() as logs:
         identify.get_operator_artifact_type(yaml)
 
     logs.check(('operatorcourier.identify',