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

terjekv · 2023-03-29T06:04:01Z

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 · 2023-03-29T06:04:23Z

Nope, still no tests.

ponas · 2023-03-29T10:56:49Z

lgtm 👍

oyvindkolbu

Has this been tested with the mreg-cli test suite? It uses quite a lot of api calls with filtering.

Also lots and lots of unrelated changes to Django 4 / django-filter.

oyvindkolbu · 2023-03-29T14:12:30Z

tox.ini

+    CI=True
+passenv = MREG_*, GITHUB_*
+deps =
+    -r{toxinidir}/requirements-test.txt


Add -r{toxinidir}/requirements.txt and only have test depedencies in requirements-test.txt.

The main downside is that we then have to open up the django version directly in requirements.txt to test against multiple versions of django. Otherwise we run into dependency conflicts. I opted to keep the version locked for now in prod.

terjekv · 2023-03-29T23:42:37Z

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 · 2023-03-30T09:44:48Z

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 · 2023-03-30T09:51:25Z

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 · 2023-03-30T09:54:52Z

For some reason, two commits from me have ended up as part of this branch. They are already in the master branch (b09877b and 749d096) 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 · 2023-03-30T09:58:32Z

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

coveralls · 2023-03-30T10:27:24Z

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

terjekv · 2023-03-30T21:58:49Z

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

oyvindhagberg · 2023-03-31T10:24:44Z

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:

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.
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 · 2023-03-31T10:37:00Z

I propose to:

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.

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 · 2023-06-06T11:25:55Z

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 · 2023-06-06T13:04:26Z

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

terjekv · 2023-06-07T14:47:31Z

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 303999f

oyvindhagberg · 2023-06-12T11:59:01Z

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

terjekv · 2023-06-12T14:47:24Z

There we go, ready for testing.

oyvindhagberg · 2023-06-19T12:15:25Z

We have to find out why uploading to Coveralls suddenly fails... The branch is out of date anyway, so maybe rebase after #502 is merged and see if it works better then.

terjekv · 2023-06-19T18:21:02Z

@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 · 2023-06-19T22:00:49Z

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 · 2023-06-20T17:02:04Z

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 · 2023-06-20T20:01:15Z

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 · 2023-06-21T14:47:52Z

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 ( unioslo/mreg-cli#168 ), will look at the mreg workflow.

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 take place in the following views: HostList. Relies on manipulating the queryset, and said manipulation is also used in the filter-less HostDetail. *Zone*List. 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.

- Required as we intriduced the toml file for ruff. - Sets some default keys. - Make tox.ini use the lint and coverage environments. - Make tox runs that use pythons and django show versions for both.

- Uses filterset_class correctly. - Applies the filter via MregMixin. - Test basic filtering in hostgroups.

- Requires manual lookup declarations. - See unioslo#489 (comment)

oyvindhagberg

Recently switched to trunk based development so the master branch is now considered development.

terjekv changed the title ~~Fixes #487 by implementing option 5 in the issue.~~ Django 4 support: Option 5, migrating to django-filter Mar 29, 2023

terjekv changed the title ~~Django 4 support: Option 5, migrating to django-filter~~ Django 4 support: Option 5, migrating to django-filter (v2, trying to get tests to run) Mar 29, 2023

terjekv mentioned this pull request Mar 29, 2023

Django 4 support: Option 5, migrating to django-filter #491

Closed

terjekv changed the title ~~Django 4 support: Option 5, migrating to django-filter (v2, trying to get tests to run)~~ Django 4 support: Option 5, migrating to django-filter (v2) Mar 29, 2023

oyvindkolbu reviewed Mar 29, 2023

View reviewed changes

terjekv force-pushed the migrate-to-django-filter-v2 branch from d5e3d56 to ae7fc74 Compare March 30, 2023 09:56

terjekv force-pushed the migrate-to-django-filter-v2 branch from ae7fc74 to 744946e Compare March 30, 2023 10:08

terjekv force-pushed the migrate-to-django-filter-v2 branch from 69bd357 to f90e036 Compare May 23, 2023 19:05

terjekv self-assigned this May 23, 2023

terjekv force-pushed the migrate-to-django-filter-v2 branch from f90e036 to 61a57fa Compare May 26, 2023 10:12

terjekv force-pushed the migrate-to-django-filter-v2 branch from 61a57fa to 303999f Compare June 7, 2023 14:46

terjekv mentioned this pull request Jun 7, 2023

Django 4 support: Option 3/4, using dj-url-filter. #488

Closed

terjekv force-pushed the migrate-to-django-filter-v2 branch from 6b99cd4 to e11bd45 Compare June 12, 2023 14:24

terjekv mentioned this pull request Jun 12, 2023

Multi-python testing via tox #502

Merged

terjekv force-pushed the migrate-to-django-filter-v2 branch from e11bd45 to 540d5bd Compare June 19, 2023 17:58

terjekv mentioned this pull request Jun 27, 2023

Django 5 deprecates CiCharField. #489

Closed

terjekv force-pushed the migrate-to-django-filter-v2 branch from 6df5a61 to 6ee17bc Compare August 18, 2023 08:05

terjekv force-pushed the migrate-to-django-filter-v2 branch from 6ee17bc to a01293e Compare August 28, 2023 13:35

terjekv force-pushed the migrate-to-django-filter-v2 branch from a01293e to 5495978 Compare September 18, 2023 07:07

terjekv force-pushed the migrate-to-django-filter-v2 branch from 5495978 to d1fcbf5 Compare November 29, 2023 13:09

terjekv requested review from oyvindhagberg and pederhan November 29, 2023 13:12

terjekv added 7 commits December 5, 2023 09:35

Update pyproject.toml fix package building (for tox)

aa9c83d

- Required as we intriduced the toml file for ruff. - Sets some default keys. - Make tox.ini use the lint and coverage environments. - Make tox runs that use pythons and django show versions for both.

A few cleanup fixes.

3d2a97f

- Uses filterset_class correctly. - Applies the filter via MregMixin. - Test basic filtering in hostgroups.

Hack support CI-fields.

c3a469e

- Requires manual lookup declarations. - See unioslo#489 (comment)

Rebase to master, clean up imports a bit.

e6ed647

Cleanup after rebase.

454a07b

Remove unused variable.

bd4fe41

terjekv force-pushed the migrate-to-django-filter-v2 branch from d1fcbf5 to bd4fe41 Compare December 5, 2023 08:35

terjekv added 2 commits April 22, 2024 17:51

Merge branch 'master' into migrate-to-django-filter-v2

630faf8

Merge branch 'master' into migrate-to-django-filter-v2

34d5032

oyvindhagberg approved these changes May 16, 2024

View reviewed changes

pederhan merged commit 0decba3 into unioslo:master May 21, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

terjekv commented Mar 29, 2023 •

edited

Loading

terjekv commented Mar 29, 2023

ponas commented Mar 29, 2023

oyvindkolbu left a comment

oyvindkolbu Mar 29, 2023

terjekv Mar 29, 2023 •

edited

Loading

terjekv commented Mar 29, 2023 •

edited

Loading

oyvindhagberg commented Mar 30, 2023

terjekv commented Mar 30, 2023

oyvindhagberg commented Mar 30, 2023

terjekv commented Mar 30, 2023

coveralls commented Mar 30, 2023 •

edited

Loading

terjekv commented Mar 30, 2023

oyvindhagberg commented Mar 31, 2023

terjekv commented Mar 31, 2023

oyvindhagberg commented Jun 6, 2023

terjekv commented Jun 6, 2023

terjekv commented Jun 7, 2023

oyvindhagberg commented Jun 12, 2023

terjekv commented Jun 12, 2023

oyvindhagberg commented Jun 19, 2023

terjekv commented Jun 19, 2023 •

edited

Loading

terjekv commented Jun 19, 2023

oyvindhagberg commented Jun 20, 2023

terjekv commented Jun 20, 2023 •

edited

Loading

oyvindhagberg commented Jun 21, 2023

oyvindhagberg left a comment

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

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

Conversation

terjekv commented Mar 29, 2023 • edited Loading

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

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

Concerns:

terjekv commented Mar 29, 2023

ponas commented Mar 29, 2023

oyvindkolbu left a comment

Choose a reason for hiding this comment

oyvindkolbu Mar 29, 2023

Choose a reason for hiding this comment

terjekv Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

terjekv commented Mar 29, 2023 • edited Loading

oyvindhagberg commented Mar 30, 2023

terjekv commented Mar 30, 2023

oyvindhagberg commented Mar 30, 2023

terjekv commented Mar 30, 2023

coveralls commented Mar 30, 2023 • edited Loading

terjekv commented Mar 30, 2023

oyvindhagberg commented Mar 31, 2023

terjekv commented Mar 31, 2023

oyvindhagberg commented Jun 6, 2023

terjekv commented Jun 6, 2023

terjekv commented Jun 7, 2023

oyvindhagberg commented Jun 12, 2023

terjekv commented Jun 12, 2023

oyvindhagberg commented Jun 19, 2023

terjekv commented Jun 19, 2023 • edited Loading

terjekv commented Jun 19, 2023

oyvindhagberg commented Jun 20, 2023

terjekv commented Jun 20, 2023 • edited Loading

oyvindhagberg commented Jun 21, 2023

oyvindhagberg left a comment

Choose a reason for hiding this comment

terjekv commented Mar 29, 2023 •

edited

Loading

terjekv Mar 29, 2023 •

edited

Loading

terjekv commented Mar 29, 2023 •

edited

Loading

coveralls commented Mar 30, 2023 •

edited

Loading

terjekv commented Jun 19, 2023 •

edited

Loading

terjekv commented Jun 20, 2023 •

edited

Loading