Functional changes to use the concrete job model where appropriate #1457

glennmatthews · 2022-03-07T21:35:57Z

Relates-to: #1001

REST API changes:

Added /api/extras/job-models/ URL pattern; existing /api/extras/jobs/ should be considered deprecated
- As we've discussed, once we have a decision on how we want to formally begin using REST API versioning, this may change.
This viewset supports listing, retrieving, updating, bulk-updating, deleting, and bulk-deleting Job records, but not creating or bulk-creating them (same pattern as the UI)
The new /api/extras/job-models/<pk>/run API endpoint accepts the same parameters as the existing job-running API endpoint, but instead of returning a serialized job-class instance (why?) now returns either a serialized JobResult or a serialized ScheduledJob depending on which action resulted from the request.
- Open to discussion as to whether we should instead split this out into a pair of endpoints, i.e. one for run-immediately and one for schedule/submit-for-approval; the current implementation was basically the lowest-effort way to get a working v2 API endpoint.
Also, as GET api/extras/job-models/<pk> returns a serialized JobModel, not a serialized JobClass, this endpoint does not return information about the parameters expected on job submission as that's a JobClass property. To fill this gap, I added an api/extras/job-models/<pk>/variables query endpoint which returns a detailed view into the job variables.
- I just realized I didn't add any unit test automation for this, whoops!
The JobResult and ScheduledJob serializers/API endpoints now include a NestedJobSerializer representation of the associated JobModel.
Both the existing JobClass and new JobModel REST API viewsets now properly enforce object-based permissions on JobModel when retrieving, editing, deleting, and running Jobs.
I don't think the ScheduledJob approval/dry-run REST API endpoints currently enforce permissions properly, I need to look into this and add some test coverage (opened [1.3] Scheduled-job approval/dry-run REST API doesn't enforce object-based permissions yet #1478 to cover this gap)

UI changes:

JobResult and ScheduledJob tables now can be filtered by associated JobModel
JobClass.as_form() now derives its commit_default and read_only properties from the JobModel rather than the JobClass
Job form view for submitting or scheduling a job (job.html) derives its name, description, read_only and approval_required properties from the JobModel rather than the JobClass.
- Additionally it warns the user and disables the submit button if the JobModel is not both installed and enabled.
Job approval form derives its name, description, read_only properties from the JobModel rather than the JobClass
- Additionally it warns the user if the JobModel is not both installed and enabled
- Need to add logic to disable the dry-run and approve buttons if the job is not runnable
JobResult detail view pulls grouping, name, and description from the JobModel if available (with a fallback to the JobClass available for now)

Functional changes:

run_job refuses to run if the associated JobModel is not both installed and enabled.
run_job derives its soft_time_limit, time_limit, and read_only properties from the JobModel rather than the JobClass
JobResult.enqueue_job pulls the soft_time_limit and time_limit from the JobModel rather than the JobClass.
Added .get_for_class_path convenience method to the JobModel manager.
Job and job approval UI views pull information from the JobModel instead of the JobClass as appropriate
- Object-based permissions are now enforced for the run job UI view
- Need to enforce and test object-based permissions for approving a scheduled job

Database changes:

Added a git_repository foreign-key on JobModel, Jobs derived from a Git repository now have source = "git" and an appropriately set foreign-key, instead of the previous behavior of source = "git.{repository_slug}".
Added appropriate schema and data migrations.
Added clean logic on JobModel to ensure that field length limits are enforced for the fields that are auto-populated from JobClass code.

Testing changes:

All existing JobClass running API tests in test_api.py are now also executed against the new JobModel run API.
- Added API tests for object-based permissions in both run-job API views
- Added API tests for running a non-installed JobModel in both API views
- Added standard/generic API tests for the new list/retrieve/update/delete APIs
- As noted above I need to add unit tests for the /variables API endpoint still
Added logic in test_jobs.py to ensure that the JobModel is enabled when performing various job-running tests.
- Also did a bunch of refactoring here to eliminate a bunch of repeated boilerplate
test_models.py adds some tests on input validation when generating a JobModel.
Refactored test_scripts.py because inline declaration of Script (JobClass) classes within a test case don't work so well when we need a JobModel associated with them.
test_views.py runs all of its tests against both the existing class-path-based URL patterns as well as the new slug-based URL patterns for jobs
- Added tests for object-based permissions as well.

…where appropriate

jathanism

I don't know how you do it, but another large body of work that requires little input. I just have a couple comments, but nothing deal-breaking!

nautobot/extras/querysets.py

jathanism · 2022-03-08T00:40:48Z

nautobot/extras/tests/test_api.py

+    def _get_detail_url(self, instance):
+        """
+        Override default implementation as we're under "jobmodel-detail" rather than "job-detail".
+        """
+        viewname = f"{self._get_view_namespace()}:jobmodel-detail"
+        return reverse(viewname, kwargs={"pk": instance.pk})
+
+    def _get_list_url(self):
+        """
+        Override default implementation as we're under "jobmodel-list" rather than "job-list".
+        """
+        viewname = f"{self._get_view_namespace()}:jobmodel-list"
+        return reverse(viewname)


These rout names just gave me pause. Are these non-standard URL route names going to break some asserted patterns with helpers/utilities/templatetags/etc. where the singular is expected to match the model name?

Quite possibly, yes. The problem is that for the REST API, the job-* URL patterns are already in use by the existing (JobClass-based) API views. I could rename those and reclaim those patterns for the new API views but I was concerned about the possibility of breaking plugins by doing so. Happy to discuss further.

To be revisited as part of #1465 I think.

jathanism · 2022-03-08T00:42:56Z

nautobot/extras/tests/test_api.py

+        self.assertIn("schedule", response.data)
+        self.assertIn("job_result", response.data)
+        self.assertIsNone(response.data["schedule"])
+        # The urls in a NestedJobResultSerializer depends on the request context, which we don't have


Could you fake a request with django.test.RequestFactory? This comment helps but it's not entirely clear why deleting the url fields is necessary otherwise. (It's not a big deal, just more of a curiosity.)

I'll look into this. Thanks!

I tried some various approaches using DRF's APIRequestFactory but couldn't figure out the magic sauce to make it work - I kept getting 403 Forbidden responses even when explicitly using force_authenticate(request). Leaving this as-is for now.

glennmatthews · 2022-03-08T17:23:36Z

Additional TODOs (opened #1479 for these)

the Job documentation needs to be updated to explain how JobModel overrides JobClass
the Job documentation needs to be updated for the new REST API

bryanculver

A few nits but looks good overall.

nautobot/docs/release-notes/version-1.3.md

nautobot/extras/api/views.py

bryanculver · 2022-03-10T15:10:25Z

nautobot/extras/api/views.py

+    job_content_type = ContentType.objects.get(app_label="extras", model="job")
+
+    schedule_data = input_serializer.data.get("schedule")
+    # BUG TODO: it looks like by omitting "schedule" you can immediately run an approval_required job here...


If this is the case, worthwhile backporting the fix to this.

Filed #1476 to investigate this and fix it if needed.

bryanculver · 2022-03-10T15:12:24Z

nautobot/extras/views.py

                job_content_type,
-                scheduled_job.user,


Should remove this as a bug fix to current release.

Opened #1475 for this.

nautobot/extras/templates/extras/job.html

bryanculver · 2022-03-10T15:35:43Z

nautobot/extras/templates/extras/job.html

@@ -82,22 +92,30 @@ <h1>{{ job }}</h1>
                            {% endif %}
                        </div>
                        <div class="pull-right">
-                            <button type="submit" name="_run" id="id__run" class="btn btn-primary"{% if not perms.extras.run_job %} disabled="disabled"{% endif %}><i class="mdi mdi-play"></i> Run Job Now</button>
+                            <button type="submit" name="_run" id="id__run" class="btn btn-primary"
+                                {% if not perms.extras.run_job or not job_model.installed or not job_model.enabled or job_model.job_class is None %}


Maybe future enhancement:

can_i_run(user): return bool, reasons

For now I added a JobModel.runnable property that just aliases (installed and enabled and job_class is not None) as a step in the right direction.

Perfect! Just corroborating the job_model introspection would be better as a template variable than a string of conditions inside the template logic and that solves it concisely.

nautobot/extras/tests/test_api.py

…rovalRequestView

glennmatthews added 25 commits March 7, 2022 15:28

Improve Job and JobResult views to use JobModel rather than JobClass …

e9ebb47

…where appropriate

Refine views, improve templates, add testing

3097097

Black

6968350

More migration from job class to job model

c52a083

Update JobResult detail view

effd6b8

API updates - WIP

0336990

API updates and testing - WIP

0afdbd8

Update Jobs when changing GitRepository slug

ebff35c

More validation and testing

6dc979d

Cleanup

449be03

Test fixups

4e4e805

Enforce installed/enabled in run_job function, update tests accordingly

ff84372

Refactoring

ba8c88b

Flake8

262c5f3

Improve response from new REST API run endpoint

a32c413

Add note about job enablement defaults

7310dac

Add API tests for new run-job endpoint, including fixing its permissions

368fe01

Add permissions tests for running jobs via the new API

e9e6fbe

Enforce object-level permissions on legacy Job API views as well

dcfd484

Revert as_form to job_form change

9bedc8f

Remove no longer needed import

c6d719e

Add note about possible change to REST API URL

3f35552

Add test coverage for run job UI views

77f6c0c

Add a proper foreign key for job-to-git-repository

506c116

Remove unneeded code

9f6641e

jathanism approved these changes Mar 8, 2022

View reviewed changes

Add test case for API /variables/ endpoint

4bc54d6

bryanculver approved these changes Mar 10, 2022

View reviewed changes

Implement job approval permissions in the UI and add tests for JobApp…

7c4e474

…rovalRequestView

glennmatthews mentioned this pull request Mar 10, 2022

Job approval dry-run calls enqueue_job with incorrect parameters #1475

Closed

Address review feedback

4b16164

This was referenced Mar 11, 2022

[1.3] Scheduled-job approval/dry-run REST API doesn't enforce object-based permissions yet #1478

Closed

[1.3] Jobs documentation needs to be updated #1479

Closed

glennmatthews merged commit 8c42ce5 into next Mar 11, 2022

glennmatthews deleted the gfm-job-model-functional-changes branch March 11, 2022 16:26

glennmatthews added a commit that referenced this pull request Mar 11, 2022

Add release-note for #1457

a7bbff7

glennmatthews mentioned this pull request Mar 11, 2022

Dynamic Groups feature (phase 1) #1425

Merged

bryanculver added this to the v1.3.0 milestone Mar 15, 2022

glennmatthews mentioned this pull request Mar 21, 2022

Explore adding a concrete Job model #1001

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Functional changes to use the concrete job model where appropriate #1457

Functional changes to use the concrete job model where appropriate #1457

glennmatthews commented Mar 7, 2022 •

edited

Loading

jathanism left a comment

jathanism Mar 8, 2022

glennmatthews Mar 8, 2022

glennmatthews Mar 11, 2022

jathanism Mar 8, 2022

glennmatthews Mar 8, 2022

glennmatthews Mar 11, 2022

glennmatthews commented Mar 8, 2022 •

edited

Loading

bryanculver left a comment

bryanculver Mar 10, 2022

glennmatthews Mar 10, 2022

bryanculver Mar 10, 2022

glennmatthews Mar 10, 2022

bryanculver Mar 10, 2022

glennmatthews Mar 10, 2022

jathanism Mar 11, 2022

Functional changes to use the concrete job model where appropriate #1457

Functional changes to use the concrete job model where appropriate #1457

Conversation

glennmatthews commented Mar 7, 2022 • edited Loading

Relates-to: #1001

jathanism left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennmatthews commented Mar 8, 2022 • edited Loading

bryanculver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennmatthews commented Mar 7, 2022 •

edited

Loading

glennmatthews commented Mar 8, 2022 •

edited

Loading