Django 4 support: Option 5, migrating to django-filter (v2)

[terjekv]

Fixes #487 by implementing option 5 in the issue.

Implements django-filter as a replacement django-url-filter

Supports: python3.7+, django 3.2+. Tests included for this range. Breaking changes: None

This implementation uses django-filter as intended. It sets a default filter backend and uses standard filter_class models where possible.

Exceptions to this implementation rule take place in the following views:

• HostList. Relies on manipulating the queryset, and said manipulation is also used in the filter-less HostDetail.
• *ZoneList. The abstraction model for these views is based around propagating a filterset into the parent class.

Concerns:

This patch creates explicit mapping filters for JSONField and CIDRField. It is unclear how well-tested these are.

(This is a replacement for #491)

[terjekv]

Nope, still no tests.

[ponas]

lgtm 👍

[terjekv]

To resolve the issue of tests not running, there was a suggestion to merge in from master, but for some reason it became individual commits and not a merge commit. :(

And yeah, testing against mreg-cli is a good idea.

[oyvindhagberg]

To resolve the issue of tests not running, there was a suggestion to merge in from master, but for some reason it became individual commits and not a merge commit. :(

I prefer that you don't merge in from master, instead rebase the feature branch on top of master.

[terjekv]

I prefer that as well. I'll look at cleaning this up. I'm honestly not sure what happened. There was a conflict and continue really didn't do what I expected. I'll rebase and reapply.

[oyvindhagberg]

For some reason, two commits from me have ended up as part of this branch. They are already in the master branch (b09877b2951f740727c550f65259298429ca3bc5 and 749d0968b034ee982c91ee17d37df3d04e3262c7) so they can safely be removed from this branch. @terjekv I volunteer to pull this branch from your repo into the main repo, and rebase it and remove the unnecessary commits at the same time. That would probably fix the workflow trigger problem we've been struggling with too. Let me know if that's okay

[terjekv]

@oyvindhagberg I may just have broken that by doing a clean rebase and force push. :(

[coveralls]

coverage: 98.998% (-0.2%) from 99.164% when pulling 34d50326336b9452bbd669f6f91141ba4862293c on terjekv:migrate-to-django-filter-v2 into aec618ac4258a2de6339d15d1ef1b3473668e619 on unioslo:master.

[terjekv]

The JSON interface in particular needs testing before this can get anywhere.

[oyvindhagberg]

Regarding mreg-cli, I've been trying to run the testsuite locally against the container image artifact. Once I got it running, I got a crash in mreg-cli, actually. It's hard to know whether that's a bug in mreg-cli, or mreg, or somehow the environment isn't correctly set up.

mreg contains a plethora of unit tests, so in my opinion that should be sufficient, but of course the more tests the better.

I propose to:

1. Modify the mreg-cli testing framework to run against an actual mreg server instance, instead of mocking the server responses. The goal is to verify both that mreg-cli is sending the correct API calls, and that mreg is responding as expected.
2. Add a step in the CI workflow for mreg that pulls mreg-cli and runs the testsuite

It will be a while before I can carve out some time to do this. It's up to you whether this PR has to wait for that or not.

[terjekv]

I propose to:

1. Modify the mreg-cli testing framework to run against an actual mreg server instance, instead of mocking the server responses. The goal is to verify both that mreg-cli is sending the correct API calls, and that mreg is responding as expected.
2. Add a step in the CI workflow for mreg that pulls mreg-cli and runs the testsuite

I was thinking the same. This is a very very good idea. The PR can wait. There are fairly fundamental changes involved.

[oyvindhagberg]

Finally, we have the new mreg-cli testing regime in place, and I've been able to run the test suite (a list of mreg-cli commands) towards this branch of mreg and compared the results.

That uncovered a bug: It seems the API endpoint for hostgroups returns a different group than what was asked for. Perhaps the filter doesn't work. Here's an excerpt from a log that shows the http request and response:

{
    "method": "GET",
    "url": "/api/v1/hostgroups/?name=yourgroup",
    "data": {},
    "status": 200,
    "response": {
      "count": 1,
      "next": null,
      "previous": null,
      "results": [
        {
          "parent": [],
          "groups": [],
          "hosts": [
            {
              "name": "testhost1.example.org"
            }
          ],
          "owners": [
            {
              "name": "myself"
            }
          ],
          "name": "mygroup",
          "description": "This describes the group"
        }
      ]
    }
  },

[terjekv]

Huh. That was interesting. I'll have a look at it, great work!

[terjekv]

Finally, we have the new mreg-cli testing regime in place, and I've been able to run the test suite (a list of mreg-cli commands) towards this branch of mreg and compared the results.

That uncovered a bug: It seems the API endpoint for hostgroups returns a different group than what was asked for. Perhaps the filter doesn't work. Here's an excerpt from a log that shows the http request and response:

{
    "method": "GET",
    "url": "/api/v1/hostgroups/?name=yourgroup",
    "data": {},
    "status": 200,
    "response": {
      "count": 1,
      "next": null,
      "previous": null,
      "results": [
        {
          "parent": [],
          "groups": [],
          "hosts": [
            {
              "name": "testhost1.example.org"
            }
          ],
          "owners": [
            {
              "name": "myself"
            }
          ],
          "name": "mygroup",
          "description": "This describes the group"
        }
      ]
    }
  },

Should be fixed in https://github.com/unioslo/mreg/pull/492/commits/303999fdd730ddd575ed2c73b9d2da9fb651a59c

[oyvindhagberg]

Couple of conflicts now after the previous PR was merged. If you fix those I can run the mreg-cli tests again

[terjekv]

There we go, ready for testing.

[oyvindhagberg]

We have to find out why uploading to Coveralls suddenly fails... The branch is out of date anyway, so maybe rebase after https://github.com/unioslo/mreg/pull/502 is merged and see if it works better then.

[terjekv]

@oyvindhagberg Huh. This was fascinating. Any idea what is going on here? https://github.com/unioslo/mreg/actions/runs/5314852531/jobs/9622678184?pr=492#step:7:4567

[terjekv]

It looks like https://github.com/unioslo/mreg-cli/blob/51594309f34385c85940c248243f6343823efc77/mreg_cli/dhcp.py#L39 assumes we get at least one IP address. That kind of makes sense, as mreg shouldn't give it broken data, but mreg-cli shouldn't crash either way.

[oyvindhagberg]

It looks like https://github.com/unioslo/mreg-cli/blob/51594309f34385c85940c248243f6343823efc77/mreg_cli/dhcp.py#L39 assumes we get at least one IP address. That kind of makes sense, as mreg shouldn't give it broken data, but mreg-cli shouldn't crash either way.

I'll try and fix that in mreg-cli so we can run the tests again.

[terjekv]

It looks like https://github.com/unioslo/mreg-cli/blob/51594309f34385c85940c248243f6343823efc77/mreg_cli/dhcp.py#L39 assumes we get at least one IP address. That kind of makes sense, as mreg shouldn't give it broken data, but mreg-cli shouldn't crash either way.

I'll try and fix that in mreg-cli so we can run the tests again.

I'm honestly finding it a bit hard follow the mreg-cli tests. I'd love to be able to see something like this (example only, this isn't a valid flow):

1. host add foo [OK]
2. host info foo [OK]
3. host add foo [FAILURE]:
 3.1. GET /api/v1/hosts/foo -> 404 Not found 
 3.1. POST /api/v1/hosts { name: "foo" } -> 403 Forbidden
4. host delete foo [FAILURE]:
 4.1. DELETE /api/v1/hosts/foo -> 404 Not found
...

We may also have to add some information about the user performing the action.

This would make it a lot easier to follow failing tests and find the source of the failure. Here we see the the creation fails, so the delete is a continuation of the same error. This would also make it easier to "copy" over the tests to mreg. Oh, and we might also need state overview (ie, what else is in the database during a failure).

We could store the state of the system as an artifact that is parseable. Hm.

[oyvindhagberg]

I agree, the output from the mreg cli tests right now is just a byproduct of the commands that are being run, and isn't very useful. The actual useful information is found in the file new_output.json which is created by run_testsuite_and_record.sh. It contains every command being run, output made by the client, every http call made along with the http response. We should modify the workflow to output the contents of that file instead, as well as uploading it as an artifact. I've fixed the bug in mreg-cli ( https://github.com/unioslo/mreg-cli/pull/168 ), will look at the mreg workflow.