Merge pull request arachnys#558 from arachnys/refactor-check-plugin-a…

…rchitecture

Refactor check plugin architecture

cabot/cabot_config.py

@@ -31,6 +31,3 @@
 
 # Default plugins are used if the user has not specified.
 CABOT_PLUGINS_ENABLED = os.environ.get('CABOT_PLUGINS_ENABLED', 'cabot_alert_hipchat,cabot_alert_twilio,cabot_alert_email')
-
-# No custom check plugins are used if none specified
-CABOT_CUSTOM_CHECK_PLUGINS = os.environ.get('CABOT_CUSTOM_CHECK_PLUGINS', '')

cabot/cabotapp/migrations/0001_initial.py

@@ -3,28 +3,7 @@
 
 from django.db import models, migrations
 from django.conf import settings
-from cabot.settings import CABOT_CUSTOM_CHECK_PLUGINS_PARSED as CABOT_CUSTOM_CHECK_PLUGINS
-from functools import reduce
 
-def add_custom_check_plugins_models(operations, plugin_name):
-    name = plugin_name.replace('cabot_check_', '')
-    if plugin_name != '':
-        operation = migrations.CreateModel(
-            name=name.capitalize() + 'StatusCheck',
-            fields=[
-            ],
-            options={
-                'abstract': False,
-                'proxy': True,
-            },
-            bases=('cabotapp.statuscheck',),
-        )
-
-        operations += [
-            operation
-        ]
-
-    return operations
 
 class Migration(migrations.Migration):
 
@@ -33,7 +12,7 @@ class Migration(migrations.Migration):
         ('contenttypes', '0001_initial'),
     ]
 
-    operations = reduce(add_custom_check_plugins_models, [[
+    operations = [
         migrations.CreateModel(
             name='AlertAcknowledgement',
             fields=[
@@ -320,4 +299,4 @@ class Migration(migrations.Migration):
             },
             bases=('cabotapp.statuscheck',),
         ),
-    ]] + CABOT_CUSTOM_CHECK_PLUGINS)
+    ]

cabot/cabotapp/models/base.py

@@ -65,15 +65,28 @@ def calculate_debounced_passing(recent_results, debounce=0):
             return True
     return False
 
-def add_custom_check_plugins():
+def get_custom_check_plugins():
     custom_check_types = []
-    if len(settings.CABOT_CUSTOM_CHECK_PLUGINS_PARSED) > 0:
-        for plugin_name in settings.CABOT_CUSTOM_CHECK_PLUGINS_PARSED:
-            check_name = plugin_name.replace('cabot_check_', '')
-            custom_check = {}
-            custom_check['creation_url'] = "create-" + check_name + "-check"
-            custom_check['check_name'] = check_name
-            custom_check_types.append(custom_check)
+    check_subclasses = StatusCheck.__subclasses__()
+
+    # Checks that aren't using the plugin system
+    legacy_checks = [
+        "JenkinsStatusCheck",
+        "HttpStatusCheck",
+        "ICMPStatusCheck",
+        "GraphiteStatusCheck",
+    ]
+
+    for check in check_subclasses:
+        if check.__name__ in legacy_checks:
+            continue
+
+        check_name = check.check_name
+        custom_check = {}
+        custom_check['creation_url'] = "create-" + check_name + "-check"
+        custom_check['check_name'] = check_name
+        custom_check['icon_class'] = getattr(check, "icon_class", "glyphicon-ok")
+        custom_check_types.append(custom_check)
 
     return custom_check_types
 

cabot/cabotapp/tests/test_urlprefix.py

@@ -34,7 +34,6 @@ def __init__(self, urlprefix, custom_check_plugins):
             COMPRESS_URL="%s/static/" % urlprefix,
             COMPRESS_ENABLED=False,
             COMPRESS_PRECOMPILERS=(),
-            CABOT_CUSTOM_CHECK_PLUGINS_PARSED=custom_check_plugins,
             INSTALLED_APPS=installed_apps
         )
 
@@ -90,61 +89,3 @@ def test_query(self):
             response_systemstatus = self.client.get(reverse('system-status'))
 
             self.assertEqual(response_systemstatus.status_code, before_systemstatus.status_code)
-
-    def test_custom_check_plugins_urls_without_plugin(self):
-        prefix = '/test'
-        self.client.login(username=self.username, password=self.password)
-
-        with self.set_url_prefix(prefix):
-            try:
-                response = self.client.get(reverse('create-skeleton-check'))
-                self.assertEqual(response.status_code, 500)
-            except Exception as e:
-                create_error = (u"Reverse for 'create-skeleton-check' not found. "
-                    "'create-skeleton-check' is not a valid view function or pattern name.")
-                self.assertEqual(e.message, create_error)
-
-            try:
-                response = self.client.get(reverse('update-skeleton-check'))
-                self.assertEqual(response.status_code, 500)
-            except Exception as e:
-                update_error = (u"Reverse for 'update-skeleton-check' not found. "
-                    "'update-skeleton-check' is not a valid view function or pattern name.")
-                self.assertEqual(e.message, update_error)
-
-            try:
-                response = self.client.get(reverse('duplicate-skeleton-check'))
-                self.assertEqual(response.status_code, 500)
-            except Exception as e:
-                duplicate_error = (u"Reverse for 'duplicate-skeleton-check' not found. "
-                    "'duplicate-skeleton-check' is not a valid view function or pattern name.")
-                self.assertEqual(e.message, duplicate_error)
-
-    def test_custom_check_plugins_urls(self):
-        sys.modules['cabot_check_skeleton'] = import_module("cabot.cabotapp.tests.fixtures.cabot_check_skeleton")
-        prefix = '/test'
-        custom_check_plugins = ['cabot_check_skeleton']
-        self.client.login(username=self.username, password=self.password)
-
-        with set_url_prefix_and_custom_check_plugins(prefix, custom_check_plugins):
-            response = self.client.get(reverse('create-skeleton-check'))
-
-            self.assertEqual(response.status_code, 200)
-
-            response = self.client.get(reverse('update-skeleton-check', args=[1]))
-
-            self.assertEqual(response.status_code, 200)
-
-            response = self.client.get(reverse('duplicate-skeleton-check', args=[1]))
-
-            self.assertEqual(response.status_code, 302)
-
-    def test_correct_custom_checks_template(self):
-        sys.modules['cabot_check_skeleton'] = import_module("cabot.cabotapp.tests.fixtures.cabot_check_skeleton")
-        prefix = '/test'
-        custom_check_plugins = ['cabot_check_skeleton']
-        self.client.login(username=self.username, password=self.password)
-
-        with set_url_prefix_and_custom_check_plugins(prefix, custom_check_plugins):
-            response = self.client.get(reverse('checks'))
-            self.assertIn(reverse('create-skeleton-check'), response.content)

cabot/cabotapp/tests/tests_basic.py

@@ -13,7 +13,7 @@
     GraphiteStatusCheck, JenkinsStatusCheck, JenkinsConfig,
     HttpStatusCheck, ICMPStatusCheck, Service, Instance,
     StatusCheckResult, minimize_targets, ServiceStatusSnapshot,
-    add_custom_check_plugins, create_default_jenkins_config)
+    get_custom_check_plugins, create_default_jenkins_config)
 from cabot.cabotapp.calendar import get_events
 from cabot.cabotapp.views import StatusCheckReportForm
 from cabot.cabotapp import tasks
@@ -303,11 +303,7 @@ def test_alert_acknowledgement(self, fake_send_alert_update, fake_send_alert):
     @patch('cabot.cabotapp.graphite.requests.get', fake_graphite_response)
     def test_graphite_run(self):
         checkresults = self.graphite_check.statuscheckresult_set.all()
-        custom_check_types = add_custom_check_plugins()
-        self.assertEqual(len(custom_check_types), 0)
         self.assertEqual(len(checkresults), 2)
-        custom_check_types = add_custom_check_plugins()
-        self.assertEqual(len(custom_check_types), 0)
         self.graphite_check.utcnow = 1387818601 # see graphite_response.json for this magic timestamp
         self.graphite_check.run()
         checkresults = self.graphite_check.statuscheckresult_set.all()
@@ -367,8 +363,6 @@ def test_graphite_series_run(self):
     def test_graphite_empty_run(self):
         checkresults = self.graphite_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 2)
-        custom_check_types = add_custom_check_plugins()
-        self.assertEqual(len(custom_check_types), 0)
         self.graphite_check.run()
         checkresults = self.graphite_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 3)
@@ -388,8 +382,6 @@ def test_graphite_empty_run(self):
     def test_graphite_timing(self):
         checkresults = self.graphite_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 2)
-        custom_check_types = add_custom_check_plugins()
-        self.assertEqual(len(custom_check_types), 0)
         self.graphite_check.run()
         checkresults = self.graphite_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 3)
@@ -401,8 +393,6 @@ def test_jenkins_run(self, mock_get_job_status):
         mock_get_job_status.return_value = fake_jenkins_response()
         checkresults = self.jenkins_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 0)
-        custom_check_types = add_custom_check_plugins()
-        self.assertEqual(len(custom_check_types), 0)
         self.jenkins_check.run()
         checkresults = self.jenkins_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 1)
@@ -413,8 +403,6 @@ def test_jenkins_blocked_build(self, mock_get_job_status):
         mock_get_job_status.return_value = jenkins_blocked_response()
         checkresults = self.jenkins_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 0)
-        custom_check_types = add_custom_check_plugins()
-        self.assertEqual(len(custom_check_types), 0)
         self.jenkins_check.run()
         checkresults = self.jenkins_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 1)
@@ -425,8 +413,6 @@ def test_timeout_handling_in_jenkins(self):
         """This works because we are effectively patching requests.get globally, including in jenkinsapi."""
         checkresults = self.jenkins_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 0)
-        custom_check_types = add_custom_check_plugins()
-        self.assertEqual(len(custom_check_types), 0)
         self.jenkins_check.run()
         checkresults = self.jenkins_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 1)
@@ -438,8 +424,6 @@ def test_timeout_handling_in_jenkins(self):
     def test_http_run(self):
         checkresults = self.http_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 0)
-        custom_check_types = add_custom_check_plugins()
-        self.assertEqual(len(custom_check_types), 0)
         self.http_check.run()
         checkresults = self.http_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 1)
@@ -464,8 +448,6 @@ def test_http_run(self):
     def test_timeout_handling_in_http(self):
         checkresults = self.http_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 0)
-        custom_check_types = add_custom_check_plugins()
-        self.assertEqual(len(custom_check_types), 0)
         self.http_check.run()
         checkresults = self.http_check.statuscheckresult_set.all()
         self.assertEqual(len(checkresults), 1)
@@ -1260,16 +1242,3 @@ def test_same_prefix_and_suffix(self):
         result = minimize_targets(["prefix.prefix.a.suffix.suffix",
                                    "prefix.prefix.b.suffix.suffix",])
         self.assertEqual(result, ["a", "b"])
-
-class TestCustomCheckPluginFunctions(LocalTestCase):
-    def test_without_check_plugins(self):
-        result = add_custom_check_plugins()
-        self.assertEqual(result, [])
-
-    @override_settings(CABOT_CUSTOM_CHECK_PLUGINS_PARSED=['cabot_check_skeleton'])
-    def test_with_check_plugins(self):
-        result = add_custom_check_plugins()
-        self.assertEqual(result, [{
-            'creation_url': 'create-skeleton-check',
-            'check_name': 'skeleton'
-        }])

cabot/cabotapp/views.py

@@ -30,7 +30,7 @@
 from models import (
     StatusCheck, GraphiteStatusCheck, JenkinsStatusCheck, HttpStatusCheck, ICMPStatusCheck,
     StatusCheckResult, UserProfile, Service, Instance, Shift, get_duty_officers,
-    add_custom_check_plugins)
+    get_custom_check_plugins)
 from tasks import run_status_check as _run_status_check
 from .graphite import get_data, get_matching_metrics
 
@@ -508,7 +508,7 @@ def render_to_response(self, context, *args, **kwargs):
         context = super(StatusCheckListView, self).get_context_data(**kwargs)
         if context is None:
             context = {}
-        context['custom_check_types'] = add_custom_check_plugins()
+        context['custom_check_types'] = get_custom_check_plugins()
         context['checks'] = StatusCheck.objects.all().order_by('name').prefetch_related('service_set', 'instance_set')
         return super(StatusCheckListView, self).render_to_response(context, *args, **kwargs)
 
@@ -528,7 +528,7 @@ class StatusCheckDetailView(LoginRequiredMixin, DetailView):
     def render_to_response(self, context, *args, **kwargs):
         if context is None:
             context = {}
-        context['custom_check_types'] = add_custom_check_plugins()
+        context['custom_check_types'] = get_custom_check_plugins()
         context['checkresults'] = self.object.statuscheckresult_set.order_by(
             '-time_complete')[:100]
         return super(StatusCheckDetailView, self).render_to_response(context, *args, **kwargs)
@@ -822,7 +822,7 @@ class InstanceDetailView(LoginRequiredMixin, DetailView):
     def get_context_data(self, **kwargs):
         context = super(InstanceDetailView, self).get_context_data(**kwargs)
         date_from = date.today() - relativedelta(day=1)
-        context['custom_check_types'] = add_custom_check_plugins()
+        context['custom_check_types'] = get_custom_check_plugins()
         context['report_form'] = StatusCheckReportForm(initial={
             'checks': self.object.status_checks.all(),
             'service': self.object,
@@ -839,7 +839,7 @@ class ServiceDetailView(LoginRequiredMixin, DetailView):
     def get_context_data(self, **kwargs):
         context = super(ServiceDetailView, self).get_context_data(**kwargs)
         date_from = date.today() - relativedelta(day=1)
-        context['custom_check_types'] = add_custom_check_plugins()
+        context['custom_check_types'] = get_custom_check_plugins()
         context['report_form'] = StatusCheckReportForm(initial={
             'alerts': self.object.alerts.all(),
             'checks': self.object.status_checks.all(),

cabot/settings.py

@@ -156,16 +156,7 @@
     exploded = re.split(r'[<>=]+', plugin)
     CABOT_PLUGINS_ENABLED_PARSED.append(exploded[0])
 
-
-CABOT_CUSTOM_CHECK_PLUGINS_PARSED = []
-for plugin in CABOT_CUSTOM_CHECK_PLUGINS.split(","):
-    exploded = re.split(r'[<>=]+', plugin)
-    CABOT_CUSTOM_CHECK_PLUGINS_PARSED.append(exploded[0])
-    CABOT_CUSTOM_CHECK_PLUGINS_PARSED = filter(lambda x: x != '', CABOT_CUSTOM_CHECK_PLUGINS_PARSED)
-
 INSTALLED_APPS += tuple(CABOT_PLUGINS_ENABLED_PARSED)
-INSTALLED_APPS += tuple(CABOT_CUSTOM_CHECK_PLUGINS_PARSED)
-
 
 COMPRESS_PRECOMPILERS = (
     ('text/coffeescript', 'coffee --compile --stdio'),

cabot/templates/cabotapp/_statuscheck_list.html

@@ -20,7 +20,7 @@ <h3>
       {% endif %}
       <!-- Custom check plugins buttons-->
       {% for checktype in custom_check_types %}
-        &nbsp;<a href="{% url checktype.creation_url %}?instance={{ instance.id }}&service={{ service.id }}" class="" title="Add new {{checktype.check_name|capfirst}} check"><i class="glyphicon glyphicon-plus"></i><i class="glyphicon glyphicon-ok"></i></a>
+        &nbsp;<a href="{% url checktype.creation_url %}?instance={{ instance.id }}&service={{ service.id }}" class="" title="Add new {{checktype.check_name|capfirst}} check"><i class="glyphicon glyphicon-plus"></i><i class="glyphicon {{checktype.icon_class|default:"glyphicon-ok"}}"></i></a>
       {% endfor %}
       </h3>
     </div>

cabot/urls.py

@@ -31,7 +31,6 @@
 
 from importlib import import_module
 import logging
-#from cabot.settings import CABOT_CUSTOM_CHECK_PLUGINS_PARSED as CABOT_CUSTOM_CHECK_PLUGINS
 
 logger = logging.getLogger(__name__)
 
@@ -174,32 +173,13 @@ def append_plugin_urls():
     for plugin in settings.CABOT_PLUGINS_ENABLED_PARSED:
         try:
             _module = import_module('%s.urls' % plugin)
-        except Exception as e:
+        except ImportError:
             pass
         else:
             urlpatterns += [
                 url(r'^plugins/%s/' % plugin, include('%s.urls' % plugin))
             ]
 
-    for plugin_name in settings.CABOT_CUSTOM_CHECK_PLUGINS_PARSED:
-        if plugin_name != '':
-            try:
-                plugin = import_module(plugin_name + ".plugin")
-            except Exception as e:
-                pass
-            else:
-                check_name = plugin_name.replace('cabot_check_', '')
-                createViewClass = getattr(plugin, '%sCheckCreateView' % check_name.capitalize())
-                updateViewClass = getattr(plugin, '%sCheckUpdateView' % check_name.capitalize())
-                duplicateFunction = getattr(plugin, 'duplicate_check')
-                urlpatterns += [
-                    url(r'^%scheck/create/' % check_name, view=createViewClass.as_view(),
-                       name='create-' + check_name + '-check'),
-                    url(r'^%scheck/update/(?P<pk>\d+)/' % check_name,
-                       view=updateViewClass.as_view(), name='update-' + check_name + '-check'),
-                    url(r'^%scheck/duplicate/(?P<pk>\d+)/' % check_name,
-                       view=duplicateFunction, name='duplicate-' + check_name + '-check')
-                ]
 
 append_plugin_urls()
 

conf/development.env.example

@@ -1,6 +1,5 @@
 # Plugins to be loaded at launch
 # CABOT_PLUGINS_ENABLED=cabot_alert_hipchat,cabot_alert_twilio,cabot_alert_email,cabot_alert_slack
-# CABOT_CUSTOM_CHECK_PLUGINS=cabot_check_skeleton
 
 DEBUG=t
 DATABASE_URL=postgres://postgres@db:5432/postgres