Fixing push status that has been broken since moving to a client (#80)

* Fixing push status that has been broken since moving to a client

* Adding unit test to cover the bug

Author: mtakaki

cachet_url_monitor/client.py

@@ -1,5 +1,6 @@
 #!/usr/bin/env python
 from typing import Dict
+from typing import Optional
 
 import click
 import requests
@@ -81,7 +82,7 @@ def get_default_metric_value(self, metric_id):
         else:
             raise exceptions.MetricNonexistentError(metric_id)
 
-    def get_component_status(self, component_id):
+    def get_component_status(self, component_id: int) -> Optional[status.ComponentStatus]:
         """Retrieves the current status of the given component. It will fail if the component does
         not exist or doesn't respond with the expected data.
         :return component status.
@@ -94,13 +95,13 @@ def get_component_status(self, component_id):
         else:
             raise exceptions.ComponentNonexistentError(component_id)
 
-    def push_status(self, component_id, component_status):
+    def push_status(self, component_id: int, component_status: status.ComponentStatus):
         """Pushes the status of the component to the cachet server.
         """
-        params = {'id': component_id, 'status': component_status}
+        params = {'id': component_id, 'status': component_status.value}
         return requests.put(f"{self.url}/components/{component_id}", params=params, headers=self.headers)
 
-    def push_metrics(self, metric_id, latency_time_unit, elapsed_time_in_seconds, timestamp):
+    def push_metrics(self, metric_id: int, latency_time_unit: str, elapsed_time_in_seconds: int, timestamp: int):
         """Pushes the total amount of seconds the request took to get a response from the URL.
         """
         value = latency_unit.convert_to_unit(latency_time_unit, elapsed_time_in_seconds)
@@ -122,7 +123,7 @@ def push_incident(self, status_value: status.ComponentStatus, is_public_incident
             # This is the first time the incident is being created.
             params = {'name': 'URL unavailable', 'message': message,
                       'status': status.IncidentStatus.INVESTIGATING.value,
-                      'visible': is_public_incident, 'component_id': component_id, 'component_status': status_value,
+                      'visible': is_public_incident, 'component_id': component_id, 'component_status': status_value.value,
                       'notify': True}
             return requests.post(f'{self.url}/incidents', params=params, headers=self.headers)
 

cachet_url_monitor/configuration.py

@@ -2,16 +2,15 @@
 import abc
 import copy
 import logging
-import os
 import re
 import time
+from typing import Dict
 
 import requests
 from yaml import dump
 
 import cachet_url_monitor.status as st
 from cachet_url_monitor.client import CachetClient, normalize_url
-from cachet_url_monitor.exceptions import MetricNonexistentError
 from cachet_url_monitor.status import ComponentStatus
 
 # This is the mandatory fields that must be in the configuration file in this
@@ -33,13 +32,37 @@ class Configuration(object):
     """Represents a configuration file, but it also includes the functionality
     of assessing the API and pushing the results to cachet.
     """
-
-    def __init__(self, config, endpoint_index: int):
-        self.endpoint_index: int = endpoint_index
+    endpoint_index: int
+    endpoint: str
+    client: CachetClient
+    token: str
+    current_fails: int
+    trigger_update: bool
+    headers: Dict[str, str]
+
+    endpoint_method: str
+    endpoint_url: str
+    endpoint_timeout: int
+    endpoint_header: Dict[str, str]
+
+    allowed_fails: int
+    component_id: int
+    metric_id: int
+    default_metric_value: int
+    latency_unit: str
+
+    status: ComponentStatus
+    previous_status: ComponentStatus
+
+    def __init__(self, config, endpoint_index: int, client: CachetClient, token: str):
+        self.endpoint_index = endpoint_index
         self.data = config
         self.endpoint = self.data['endpoints'][endpoint_index]
-        self.current_fails: int = 0
-        self.trigger_update: bool = True
+        self.client = client
+        self.token = token
+
+        self.current_fails = 0
+        self.trigger_update = True
 
         if 'name' not in self.endpoint:
             # We have to make this mandatory, otherwise the logs are confusing when there are multiple URLs.
@@ -54,7 +77,7 @@ def __init__(self, config, endpoint_index: int):
         self.validate()
 
         # We store the main information from the configuration file, so we don't keep reading from the data dictionary.
-        self.token = os.environ.get('CACHET_TOKEN') or self.data['cachet']['token']
+
         self.headers = {'X-Cachet-Token': self.token}
 
         self.endpoint_method = self.endpoint['method']
@@ -63,14 +86,11 @@ def __init__(self, config, endpoint_index: int):
         self.endpoint_header = self.endpoint.get('header') or None
         self.allowed_fails = self.endpoint.get('allowed_fails') or 0
 
-        self.api_url = os.environ.get('CACHET_API_URL') or self.data['cachet']['api_url']
         self.component_id = self.endpoint['component_id']
         self.metric_id = self.endpoint.get('metric_id')
 
-        self.client = CachetClient(self.api_url, self.token)
-
         if self.metric_id is not None:
-            self.default_metric_value = self.get_default_metric_value(self.metric_id)
+            self.default_metric_value = self.client.get_default_metric_value(self.metric_id)
 
         # The latency_unit configuration is not mandatory and we fallback to seconds, by default.
         self.latency_unit = self.data['cachet'].get('latency_unit') or 's'
@@ -88,15 +108,6 @@ def __init__(self, config, endpoint_index: int):
         for expectation in self.expectations:
             self.logger.info('Registered expectation: %s' % (expectation,))
 
-    def get_default_metric_value(self, metric_id):
-        """Returns default value for configured metric."""
-        get_metric_request = requests.get('%s/metrics/%s' % (self.api_url, metric_id), headers=self.headers)
-
-        if get_metric_request.ok:
-            return get_metric_request.json()['data']['default_value']
-        else:
-            raise MetricNonexistentError(metric_id)
-
     def get_action(self):
         """Retrieves the action list from the configuration. If it's empty, returns an empty list.
         :return: The list of actions, which can be an empty list.
@@ -162,7 +173,7 @@ def evaluate(self):
             status: ComponentStatus = expectation.get_status(self.request)
 
             # The greater the status is, the worse the state of the API is.
-            if status.value > self.status.value:
+            if status.value >= self.status.value:
                 self.status = status
                 self.message = expectation.get_message(self.request)
                 self.logger.info(self.message)

cachet_url_monitor/latency_unit.py

@@ -1,10 +1,11 @@
 #!/usr/bin/env python
+from typing import Dict
 
-seconds_per_unit = {"ms": 1000, "milliseconds": 1000, "s": 1, "seconds": 1, "m": float(1) / 60,
+seconds_per_unit: Dict[str, float] = {"ms": 1000, "milliseconds": 1000, "s": 1, "seconds": 1, "m": float(1) / 60,
                     "minutes": float(1) / 60, "h": float(1) / 3600, "hours": float(1) / 3600}
 
 
-def convert_to_unit(time_unit, value):
+def convert_to_unit(time_unit: str, value: float):
     """
     Will convert the given value from seconds to the given time_unit.
 

cachet_url_monitor/scheduler.py

@@ -7,6 +7,7 @@
 import schedule
 from yaml import load, SafeLoader
 
+from cachet_url_monitor.client import CachetClient
 from cachet_url_monitor.configuration import Configuration
 
 cachet_mandatory_fields = ['api_url', 'token']
@@ -105,14 +106,14 @@ def build_agent(configuration, logger):
 
 
 def validate_config():
-    if 'endpoints' not in config_file.keys():
+    if 'endpoints' not in config_data.keys():
         fatal_error('Endpoints is a mandatory field')
 
-    if config_file['endpoints'] is None:
+    if config_data['endpoints'] is None:
         fatal_error('Endpoints array can not be empty')
 
     for key in cachet_mandatory_fields:
-        if key not in config_file['cachet']:
+        if key not in config_data['cachet']:
             fatal_error('Missing cachet mandatory fields')
 
 
@@ -132,14 +133,16 @@ def fatal_error(message):
         sys.exit(1)
 
     try:
-        config_file = load(open(sys.argv[1], 'r'), SafeLoader)
+        config_data = load(open(sys.argv[1], 'r'), SafeLoader)
     except FileNotFoundError:
         logging.getLogger('cachet_url_monitor.scheduler').fatal(f'File not found: {sys.argv[1]}')
         sys.exit(1)
 
     validate_config()
 
-    for endpoint_index in range(len(config_file['endpoints'])):
-        configuration = Configuration(config_file, endpoint_index)
+    for endpoint_index in range(len(config_data['endpoints'])):
+        token = os.environ.get('CACHET_TOKEN') or config_data['cachet']['token']
+        api_url = os.environ.get('CACHET_API_URL') or config_data['cachet']['api_url']
+        configuration = Configuration(config_data, endpoint_index, CachetClient(api_url, token), token)
         NewThread(Scheduler(configuration,
                             build_agent(configuration, logging.getLogger('cachet_url_monitor.scheduler')))).start()

tests/test_client.py

@@ -145,3 +145,11 @@ def json():
 
         self.assertEqual(status, ComponentStatus.OPERATIONAL,
                          'Getting component status value is incorrect.')
+
+    @requests_mock.mock()
+    def test_push_status(self, m):
+        m.put(f'{CACHET_URL}/components/123?id=123&status={ComponentStatus.PARTIAL_OUTAGE.value}',
+              headers={'X-Cachet-Token': TOKEN})
+        response = self.client.push_status(123, ComponentStatus.PARTIAL_OUTAGE)
+
+        self.assertTrue(response.ok, 'Pushing status value is failed.')

tests/test_configuration.py

@@ -9,42 +9,35 @@
 from yaml import load, SafeLoader
 
 import cachet_url_monitor.status
+from cachet_url_monitor.client import CachetClient
+import cachet_url_monitor.exceptions
 
 sys.modules['logging'] = mock.Mock()
 from cachet_url_monitor.configuration import Configuration
 import os
 
 
 class ConfigurationTest(unittest.TestCase):
-    @mock.patch.dict(os.environ, {'CACHET_TOKEN': 'token2'})
+    client: CachetClient
+    configuration: Configuration
+
     def setUp(self):
         def getLogger(name):
             self.mock_logger = mock.Mock()
             return self.mock_logger
 
         sys.modules['logging'].getLogger = getLogger
-
-        # def get(url, headers):
-        #     get_return = mock.Mock()
-        #     get_return.ok = True
-        #     get_return.json = mock.Mock()
-        #     get_return.json.return_value = {'data': {'status': 1, 'default_value': 0.5}}
-        #     return get_return
-        #
-        # sys.modules['requests'].get = get
-
+        self.client = mock.Mock()
+        # We set the initial status to OPERATIONAL.
+        self.client.get_component_status.return_value = cachet_url_monitor.status.ComponentStatus.OPERATIONAL
         self.configuration = Configuration(
-            load(open(os.path.join(os.path.dirname(__file__), 'configs/config.yml'), 'rt'), SafeLoader), 0)
-        # sys.modules['requests'].Timeout = Timeout
-        # sys.modules['requests'].ConnectionError = ConnectionError
-        # sys.modules['requests'].HTTPError = HTTPError
+            load(open(os.path.join(os.path.dirname(__file__), 'configs/config.yml'), 'rt'), SafeLoader), 0, self.client,
+            'token2')
 
     def test_init(self):
         self.assertEqual(len(self.configuration.data), 2, 'Number of root elements in config.yml is incorrect')
         self.assertEqual(len(self.configuration.expectations), 3, 'Number of expectations read from file is incorrect')
         self.assertDictEqual(self.configuration.headers, {'X-Cachet-Token': 'token2'}, 'Header was not set correctly')
-        self.assertEqual(self.configuration.api_url, 'https://demo.cachethq.io/api/v1',
-                         'Cachet API URL was set incorrectly')
         self.assertDictEqual(self.configuration.endpoint_header, {'SOME-HEADER': 'SOME-VALUE'}, 'Header is incorrect')
 
     @requests_mock.mock()
@@ -98,31 +91,49 @@ def test_evaluate_with_http_error(self, m):
                          'Component status set incorrectly')
         self.mock_logger.exception.assert_called_with('Unexpected HTTP response')
 
-    @requests_mock.mock()
-    def test_push_status(self, m):
-        m.put('https://demo.cachethq.io/api/v1/components/1?id=1&status=1', headers={'X-Cachet-Token': 'token2'})
-        self.assertEqual(self.configuration.status, cachet_url_monitor.status.ComponentStatus.OPERATIONAL,
-                         'Incorrect component update parameters')
+    def test_push_status(self):
+        self.client.get_component_status.return_value = cachet_url_monitor.status.ComponentStatus.OPERATIONAL
+        push_status_response = mock.Mock()
+        self.client.push_status.return_value = push_status_response
+        push_status_response.ok = True
+        self.configuration.status = cachet_url_monitor.status.ComponentStatus.PARTIAL_OUTAGE
+
         self.configuration.push_status()
 
-    @requests_mock.mock()
-    def test_push_status_with_failure(self, m):
-        m.put('https://demo.cachethq.io/api/v1/components/1?id=1&status=1', headers={'X-Cachet-Token': 'token2'},
-              status_code=400)
-        self.assertEqual(self.configuration.status, cachet_url_monitor.status.ComponentStatus.OPERATIONAL,
-                         'Incorrect component update parameters')
+        self.client.push_status.assert_called_once_with(1, cachet_url_monitor.status.ComponentStatus.OPERATIONAL)
+
+    def test_push_status_with_failure(self):
+        self.client.get_component_status.return_value = cachet_url_monitor.status.ComponentStatus.OPERATIONAL
+        push_status_response = mock.Mock()
+        self.client.push_status.return_value = push_status_response
+        push_status_response.ok = False
+        self.configuration.status = cachet_url_monitor.status.ComponentStatus.PARTIAL_OUTAGE
+
+        self.configuration.push_status()
+
+        self.client.push_status.assert_called_once_with(1, cachet_url_monitor.status.ComponentStatus.OPERATIONAL)
+
+    def test_push_status_same_status(self):
+        self.client.get_component_status.return_value = cachet_url_monitor.status.ComponentStatus.OPERATIONAL
+        self.configuration.status = cachet_url_monitor.status.ComponentStatus.OPERATIONAL
+
         self.configuration.push_status()
 
+        self.client.push_status.assert_not_called()
+
 
 class ConfigurationMultipleUrlTest(unittest.TestCase):
     @mock.patch.dict(os.environ, {'CACHET_TOKEN': 'token2'})
     def setUp(self):
         config_yaml = load(open(os.path.join(os.path.dirname(__file__), 'configs/config_multiple_urls.yml'), 'rt'),
                            SafeLoader)
+        self.client = []
         self.configuration = []
 
         for index in range(len(config_yaml['endpoints'])):
-            self.configuration.append(Configuration(config_yaml, index))
+            client = mock.Mock()
+            self.client.append(client)
+            self.configuration.append(Configuration(config_yaml, index, client, 'token2'))
 
     def test_init(self):
         expected_method = ['GET', 'POST']
@@ -133,8 +144,6 @@ def test_init(self):
             self.assertEqual(len(config.data), 2, 'Number of root elements in config.yml is incorrect')
             self.assertEqual(len(config.expectations), 1, 'Number of expectations read from file is incorrect')
             self.assertDictEqual(config.headers, {'X-Cachet-Token': 'token2'}, 'Header was not set correctly')
-            self.assertEqual(config.api_url, 'https://demo.cachethq.io/api/v1',
-                             'Cachet API URL was set incorrectly')
 
             self.assertEqual(expected_method[index], config.endpoint_method)
             self.assertEqual(expected_url[index], config.endpoint_url)
@@ -146,4 +155,4 @@ def test_init(self):
         with pytest.raises(cachet_url_monitor.configuration.ConfigurationValidationError):
             self.configuration = Configuration(
                 load(open(os.path.join(os.path.dirname(__file__), 'configs/config_invalid_type.yml'), 'rt'),
-                     SafeLoader), 0)
+                     SafeLoader), 0, mock.Mock(), 'token2')