From 715592547c9dce2eaeb0575ae79f978cb842827b Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Wed, 29 Mar 2023 16:51:55 -0400 Subject: [PATCH] #12081: Script & report cleanup (#12091) * start() and terminate() methods on Job should call save() * Fix display of associated jobs * Introduce get_latest_jobs() method on JobsMixin * Update messaging when no reports/scripts exist * Catch ImportErrors when rendering report/script lists * Fix loading of nested modules * Fix URLs for nested scripts/reports --- netbox/core/models/jobs.py | 7 +- .../migrations/0091_create_managedfiles.py | 11 +- netbox/extras/models/mixins.py | 23 ++-- netbox/extras/models/reports.py | 8 +- netbox/extras/models/scripts.py | 4 +- netbox/extras/reports.py | 3 +- netbox/extras/urls.py | 10 +- netbox/extras/views.py | 35 ++---- netbox/netbox/models/features.py | 16 ++- netbox/templates/extras/report_list.html | 102 +++++++++--------- netbox/templates/extras/script_list.html | 42 ++++---- 11 files changed, 130 insertions(+), 131 deletions(-) diff --git a/netbox/core/models/jobs.py b/netbox/core/models/jobs.py index d823a941b..73bcd13d6 100644 --- a/netbox/core/models/jobs.py +++ b/netbox/core/models/jobs.py @@ -39,7 +39,8 @@ class Job(models.Model): ) object = GenericForeignKey( ct_field='object_type', - fk_field='object_id' + fk_field='object_id', + for_concrete_model=False ) name = models.CharField( max_length=200 @@ -140,7 +141,7 @@ class Job(models.Model): # Start the job self.started = timezone.now() self.status = JobStatusChoices.STATUS_RUNNING - Job.objects.filter(pk=self.pk).update(started=self.started, status=self.status) + self.save() # Handle webhooks self.trigger_webhooks(event=EVENT_JOB_START) @@ -156,7 +157,7 @@ class Job(models.Model): # Mark the job as completed self.status = status self.completed = timezone.now() - Job.objects.filter(pk=self.pk).update(status=self.status, completed=self.completed) + self.save() # Handle webhooks self.trigger_webhooks(event=EVENT_JOB_END) diff --git a/netbox/extras/migrations/0091_create_managedfiles.py b/netbox/extras/migrations/0091_create_managedfiles.py index 6921247a4..79a80821f 100644 --- a/netbox/extras/migrations/0091_create_managedfiles.py +++ b/netbox/extras/migrations/0091_create_managedfiles.py @@ -8,16 +8,9 @@ import extras.models.mixins def create_files(cls, root_name, root_path): - path_tree = [ - path for path, _, _ in os.walk(root_path) - if os.path.basename(path)[0] not in ('_', '.') - ] - - modules = list(pkgutil.iter_modules(path_tree)) + modules = list(pkgutil.iter_modules([root_path])) filenames = [] - for importer, module_name, is_pkg in modules: - if is_pkg: - continue + for importer, module_name, ispkg in modules: try: module = importer.find_module(module_name).load_module(module_name) rel_path = os.path.relpath(module.__file__, root_path) diff --git a/netbox/extras/models/mixins.py b/netbox/extras/models/mixins.py index 82b0b9983..cb1d31837 100644 --- a/netbox/extras/models/mixins.py +++ b/netbox/extras/models/mixins.py @@ -1,5 +1,5 @@ import os -from pkgutil import ModuleInfo, get_importer +from importlib.machinery import SourceFileLoader __all__ = ( 'PythonModuleMixin', @@ -12,16 +12,17 @@ class PythonModuleMixin: def path(self): return os.path.splitext(self.file_path)[0] - def get_module_info(self): - path = os.path.dirname(self.full_path) - module_name = os.path.basename(self.path) - return ModuleInfo( - module_finder=get_importer(path), - name=module_name, - ispkg=False - ) + @property + def python_name(self): + path, filename = os.path.split(self.full_path) + name = os.path.splitext(filename)[0] + if name == '__init__': + # File is a package + return os.path.basename(path) + else: + return name def get_module(self): - importer, module_name, _ = self.get_module_info() - module = importer.find_module(module_name).load_module(module_name) + loader = SourceFileLoader(self.python_name, self.full_path) + module = loader.load_module() return module diff --git a/netbox/extras/models/reports.py b/netbox/extras/models/reports.py index 66006c90f..aaa785696 100644 --- a/netbox/extras/models/reports.py +++ b/netbox/extras/models/reports.py @@ -44,6 +44,9 @@ class ReportModule(PythonModuleMixin, JobsMixin, ManagedFile): def get_absolute_url(self): return reverse('extras:report_list') + def __str__(self): + return self.python_name + @cached_property def reports(self): @@ -51,7 +54,10 @@ class ReportModule(PythonModuleMixin, JobsMixin, ManagedFile): # For child objects in submodules use the full import path w/o the root module as the name return cls.full_name.split(".", maxsplit=1)[1] - module = self.get_module() + try: + module = self.get_module() + except ImportError: + return {} reports = {} ordered = getattr(module, 'report_order', []) diff --git a/netbox/extras/models/scripts.py b/netbox/extras/models/scripts.py index f830f134c..1a7559e53 100644 --- a/netbox/extras/models/scripts.py +++ b/netbox/extras/models/scripts.py @@ -1,7 +1,6 @@ import inspect from functools import cached_property -from django.contrib.contenttypes.fields import GenericRelation from django.db import models from django.urls import reverse @@ -44,6 +43,9 @@ class ScriptModule(PythonModuleMixin, JobsMixin, ManagedFile): def get_absolute_url(self): return reverse('extras:script_list') + def __str__(self): + return self.python_name + @cached_property def scripts(self): diff --git a/netbox/extras/reports.py b/netbox/extras/reports.py index 00579f4e8..ed886f425 100644 --- a/netbox/extras/reports.py +++ b/netbox/extras/reports.py @@ -195,8 +195,6 @@ class Report(object): Run the report and save its results. Each test method will be executed in order. """ self.logger.info(f"Running report") - job.status = JobStatusChoices.STATUS_RUNNING - job.save() # Perform any post-run tasks self.pre_run() @@ -218,6 +216,7 @@ class Report(object): logger.error(f"Exception raised during report execution: {e}") job.terminate(status=JobStatusChoices.STATUS_ERRORED) finally: + job.data = self._results job.terminate() # Perform any post-run tasks diff --git a/netbox/extras/urls.py b/netbox/extras/urls.py index 400596c40..a562f5a8e 100644 --- a/netbox/extras/urls.py +++ b/netbox/extras/urls.py @@ -97,17 +97,17 @@ urlpatterns = [ path('reports/add/', views.ReportModuleCreateView.as_view(), name='reportmodule_add'), path('reports/results//', views.ReportResultView.as_view(), name='report_result'), path('reports//', include(get_model_urls('extras', 'reportmodule'))), - path('reports/./', views.ReportView.as_view(), name='report'), - path('reports/./jobs/', views.ReportJobsView.as_view(), name='report_jobs'), + path('reports///', views.ReportView.as_view(), name='report'), + path('reports///jobs/', views.ReportJobsView.as_view(), name='report_jobs'), # Scripts path('scripts/', views.ScriptListView.as_view(), name='script_list'), path('scripts/add/', views.ScriptModuleCreateView.as_view(), name='scriptmodule_add'), path('scripts/results//', views.ScriptResultView.as_view(), name='script_result'), path('scripts//', include(get_model_urls('extras', 'scriptmodule'))), - path('scripts/./', views.ScriptView.as_view(), name='script'), - path('scripts/./source/', views.ScriptSourceView.as_view(), name='script_source'), - path('scripts/./jobs/', views.ScriptJobsView.as_view(), name='script_jobs'), + path('scripts///', views.ScriptView.as_view(), name='script'), + path('scripts///source/', views.ScriptSourceView.as_view(), name='script_source'), + path('scripts///jobs/', views.ScriptJobsView.as_view(), name='script_jobs'), # Markdown path('render/markdown/', views.RenderMarkdownView.as_view(), name="render_markdown") diff --git a/netbox/extras/views.py b/netbox/extras/views.py index 65ebbfaa5..22984e9ab 100644 --- a/netbox/extras/views.py +++ b/netbox/extras/views.py @@ -819,19 +819,9 @@ class ReportListView(ContentTypePermissionRequiredMixin, View): def get(self, request): report_modules = ReportModule.objects.restrict(request.user) - report_content_type = ContentType.objects.get(app_label='extras', model='report') - jobs = { - r.name: r - for r in Job.objects.filter( - object_type=report_content_type, - status__in=JobStatusChoices.TERMINAL_STATE_CHOICES - ).order_by('name', '-created').distinct('name').defer('data') - } - return render(request, 'extras/report_list.html', { 'model': ReportModule, 'report_modules': report_modules, - 'jobs': jobs, }) @@ -843,7 +833,7 @@ class ReportView(ContentTypePermissionRequiredMixin, View): return 'extras.view_report' def get(self, request, module, name): - module = get_object_or_404(ReportModule.objects.restrict(request.user), file_path=f'{module}.py') + module = get_object_or_404(ReportModule.objects.restrict(request.user), file_path__startswith=module) report = module.reports[name]() object_type = ContentType.objects.get(app_label='extras', model='reportmodule') @@ -864,7 +854,7 @@ class ReportView(ContentTypePermissionRequiredMixin, View): if not request.user.has_perm('extras.run_report'): return HttpResponseForbidden() - module = get_object_or_404(ReportModule.objects.restrict(request.user), file_path=f'{module}.py') + module = get_object_or_404(ReportModule.objects.restrict(request.user), file_path__startswith=module) report = module.reports[name]() form = ReportForm(request.POST) @@ -903,7 +893,7 @@ class ReportJobsView(ContentTypePermissionRequiredMixin, View): return 'extras.view_report' def get(self, request, module, name): - module = get_object_or_404(ReportModule.objects.restrict(request.user), file_path=f'{module}.py') + module = get_object_or_404(ReportModule.objects.restrict(request.user), file_path__startswith=module) report = module.reports[name]() object_type = ContentType.objects.get(app_label='extras', model='reportmodule') @@ -987,19 +977,9 @@ class ScriptListView(ContentTypePermissionRequiredMixin, View): def get(self, request): script_modules = ScriptModule.objects.restrict(request.user) - script_content_type = ContentType.objects.get(app_label='extras', model='script') - jobs = { - r.name: r - for r in Job.objects.filter( - object_type=script_content_type, - status__in=JobStatusChoices.TERMINAL_STATE_CHOICES - ).order_by('name', '-created').distinct('name').defer('data') - } - return render(request, 'extras/script_list.html', { 'model': ScriptModule, 'script_modules': script_modules, - 'jobs': jobs, }) @@ -1009,7 +989,8 @@ class ScriptView(ContentTypePermissionRequiredMixin, View): return 'extras.view_script' def get(self, request, module, name): - module = get_object_or_404(ScriptModule.objects.restrict(request.user), file_path=f'{module}.py') + print(module) + module = get_object_or_404(ScriptModule.objects.restrict(request.user), file_path__startswith=module) script = module.scripts[name]() form = script.as_form(initial=normalize_querydict(request.GET)) @@ -1033,7 +1014,7 @@ class ScriptView(ContentTypePermissionRequiredMixin, View): if not request.user.has_perm('extras.run_script'): return HttpResponseForbidden() - module = get_object_or_404(ScriptModule.objects.restrict(request.user), file_path=f'{module}.py') + module = get_object_or_404(ScriptModule.objects.restrict(request.user), file_path__startswith=module) script = module.scripts[name]() form = script.as_form(request.POST, request.FILES) @@ -1070,7 +1051,7 @@ class ScriptSourceView(ContentTypePermissionRequiredMixin, View): return 'extras.view_script' def get(self, request, module, name): - module = get_object_or_404(ScriptModule.objects.restrict(request.user), file_path=f'{module}.py') + module = get_object_or_404(ScriptModule.objects.restrict(request.user), file_path__startswith=module) script = module.scripts[name]() return render(request, 'extras/script/source.html', { @@ -1086,7 +1067,7 @@ class ScriptJobsView(ContentTypePermissionRequiredMixin, View): return 'extras.view_script' def get(self, request, module, name): - module = get_object_or_404(ScriptModule.objects.restrict(request.user), file_path=f'{module}.py') + module = get_object_or_404(ScriptModule.objects.restrict(request.user), file_path__startswith=module) script = module.scripts[name]() object_type = ContentType.objects.get(app_label='extras', model='scriptmodule') diff --git a/netbox/netbox/models/features.py b/netbox/netbox/models/features.py index 5573b1abc..cf141d987 100644 --- a/netbox/netbox/models/features.py +++ b/netbox/netbox/models/features.py @@ -9,9 +9,9 @@ from django.db.models.signals import class_prepared from django.dispatch import receiver from django.utils import timezone from django.utils.translation import gettext as _ - from taggit.managers import TaggableManager +from core.choices import JobStatusChoices from extras.choices import CustomFieldVisibilityChoices, ObjectChangeActionChoices from extras.utils import is_taggable, register_features from netbox.registry import registry @@ -302,12 +302,24 @@ class JobsMixin(models.Model): jobs = GenericRelation( to='core.Job', content_type_field='object_type', - object_id_field='object_id' + object_id_field='object_id', + for_concrete_model=False ) class Meta: abstract = True + def get_latest_jobs(self): + """ + Return a dictionary mapping of the most recent jobs for this instance. + """ + return { + job.name: job + for job in self.jobs.filter( + status__in=JobStatusChoices.TERMINAL_STATE_CHOICES + ).order_by('name', '-created').distinct('name').defer('data') + } + class JournalingMixin(models.Model): """ diff --git a/netbox/templates/extras/report_list.html b/netbox/templates/extras/report_list.html index 08d26aa2d..0c27eefda 100644 --- a/netbox/templates/extras/report_list.html +++ b/netbox/templates/extras/report_list.html @@ -34,7 +34,7 @@ {% endif %} - {{ module.name|bettertitle }} + {{ module }}
{% include 'inc/sync_warning.html' with object=module %} @@ -49,56 +49,58 @@ - {% for report_name, report in module.reports.items %} - {% with last_result=jobs|get_key:report.full_name %} - - - {{ report.name }} - - {{ report.description|markdown|placeholder }} - {% if last_result %} - - {{ last_result.created|annotated_date }} - - - {% badge last_result.get_status_display last_result.get_status_color %} - - {% else %} - Never - {{ ''|placeholder }} - {% endif %} - - {% if perms.extras.run_report %} -
-
- {% csrf_token %} - -
-
- {% endif %} - - - {% for method, stats in last_result.data.items %} + {% with jobs=module.get_latest_jobs %} + {% for report_name, report in module.reports.items %} + {% with last_job=jobs|get_key:report.name %} - - {{ method }} + + {{ report.name }} - - {{ stats.success }} - {{ stats.info }} - {{ stats.warning }} - {{ stats.failure }} + {{ report.description|markdown|placeholder }} + {% if last_job %} + + {{ last_job.created|annotated_date }} + + + {% badge last_job.get_status_display last_job.get_status_color %} + + {% else %} + Never + {{ ''|placeholder }} + {% endif %} + + {% if perms.extras.run_report %} +
+
+ {% csrf_token %} + +
+
+ {% endif %} - {% endfor %} - {% endwith %} - {% endfor %} + {% for method, stats in last_job.data.items %} + + + {{ method }} + + + {{ stats.success }} + {{ stats.info }} + {{ stats.warning }} + {{ stats.failure }} + + + {% endfor %} + {% endwith %} + {% endfor %} + {% endwith %}
@@ -106,9 +108,9 @@ {% empty %} {% endfor %} diff --git a/netbox/templates/extras/script_list.html b/netbox/templates/extras/script_list.html index 010444a3c..bccbce589 100644 --- a/netbox/templates/extras/script_list.html +++ b/netbox/templates/extras/script_list.html @@ -33,7 +33,7 @@ {% endif %} - {{ module.name|bettertitle }} + {{ module }}
{% include 'inc/sync_warning.html' with object=module %} @@ -47,29 +47,31 @@ - {% for script_name, script_class in module.scripts.items %} - {% with last_result=jobs|get_key:script_class.full_name %} + {% with jobs=module.get_latest_jobs %} + {% for script_name, script_class in module.scripts.items %} - {{ script_class.name }} + {{ script_class.name }} {{ script_class.Meta.description|markdown|placeholder }} - {% if last_result %} - - {{ last_result.created|annotated_date }} - - - {% badge last_result.get_status_display last_result.get_status_color %} - - {% else %} - Never - {{ ''|placeholder }} - {% endif %} + {% with last_result=jobs|get_key:script_class.name %} + {% if last_result %} + + {{ last_result.created|annotated_date }} + + + {% badge last_result.get_status_display last_result.get_status_color %} + + {% else %} + Never + {{ ''|placeholder }} + {% endif %} + {% endwith %} - {% endwith %} - {% endfor %} + {% endfor %} + {% endwith %}
@@ -77,9 +79,9 @@ {% empty %}

No Scripts Found

- Scripts should be saved to {{ settings.SCRIPTS_ROOT }}. -
- This path can be changed by setting SCRIPTS_ROOT in NetBox's configuration. + {% if perms.extras.add_scriptmodule %} + Get started by creating a script from an uploaded file or data source. + {% endif %}
{% endfor %}