Pulp AdminModels for just about every pulp object

[alikins]

Pulp AdminModels for just about every pulp object

[pulpbot]

WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue.

[alikins]

Example screenshots

[bmbouter]

I wrote this issue up, can you add closes #7336 to your commit message?
Also can you squash all your commits
Also can you ensure that the no data is write-able or edit-able. There are two options for this: try using https://github.com/vint21h/django-read-only-admin/ or create a Meta class that all PulpModelAdmin objects would use which would set ModelAdmin.readonly_fields dynamically at class construction time.

If you can do all that I can test, review and merge.

[alikins]

Also can you ensure that the no data is write-able or edit-able. There are two options for this: try using https://github.com/vint21h/django-read-only-admin/ or create a Meta class that all PulpModelAdmin objects would use which would set ModelAdmin.readonly_fields dynamically at class construction time.

I'm not sure I understand the motivation for making the admin read-only. I'm assuming it will only be accessible to django 'admin' users (or whatever auth/authz mechanism is involved). Read only loses a lot of the utility of the tool.

[bmbouter]

Also can you ensure that the no data is write-able or edit-able. There are two options for this: try using https://github.com/vint21h/django-read-only-admin/ or create a Meta class that all PulpModelAdmin objects would use which would set ModelAdmin.readonly_fields dynamically at class construction time.

I'm not sure I understand the motivation for making the admin read-only. I'm assuming it will only be accessible to django 'admin' users (or whatever auth/authz mechanism is involved). Read only loses a lot of the utility of the tool.

Django-admin bypasses the drf serializers. Pulp heavily relies on those serializes for data correctness. I agree it's not as useful, but it's also not safe and so it's a no go for us to merge anything that isn't read-only for the model data itself.

[alikins]

Note: Currently the 'PulpModelAdmin' base class sets fields and read_only_fields to the common read_only files.

But that also means all subclasses have some fields / read_only_fields defined, so the "edit view" pages are usually not often.
If we remove those, the default ModelAdmin auto stuff will happen.

Alternatively, PulpModelAdmin could provide a clever get_fields() and get_readonly_fields() to let the subclasses still autopopulate.

(and arguably, the PulpModelAdmin base class is unnecessary at the moment, but I'd leave it as is (even empty) just as an abstraction point and since likely there will shared methods for PulpModel derived classes needed in the future)

[bmbouter]

Having the PulpModelAdmin I think is a fine idea.

We need to take this further though, we can't merge it until all the fields are read-only is the current agreement within various folks in the pulp community.

I tried using django-read-only-admin and I don't recommend it, their code 500 errored for me so I think that option is out.

One option is to have PuplModelAdmin provide a metaclass that sets readonly_fields for all fields on the model. I think that would be pretty straightforward actually.

[bmbouter]

I tried putting together a metclass implementation and I hope to continue on it tomorrow. If we had that I think this PR would be pretty much good-to-go. Without that though, it isn't.

[bmbouter]

After a slack convo, my understanding is your use case really is to have a writeable version which we can't release in pulpcore due to it bypassing the DRF serializers. One option discussed is for galaxy_NG as a plugin to carry such a PR, so I think closing this here and reopening against galaxy_ng is the next step.

[alikins]

@bmbouter

After a slack convo, my understanding is your use case really is to have a writeable version which we can't release in pulpcore due to it bypassing the DRF serializers. One option discussed is for galaxy_NG as a plugin to carry such a PR, so I think closing this here and reopening against galaxy_ng is the next step.

That was my initial plan, but split the admin.py models to live in their traditional places (ie, pulpcore models in pulpcore/app/admin.py instead of in galaxy_ng/app/admin.py).

Since django-admin is the easiest way to check and verify the guardian per class / per instance permissions I was under the impression that having django-admin for pulp objects was a requirement for pulpcore itself.

[alikins]

re model validation and drf

one option may be to use a custom MyDRFModelForm for the base ModelAdmin class.
MyDRFModelForm would provide a custom .clean() that instead of calling just the django Model.clean()
validation, but could also in theory call the DRF validation.

For the latter, that assumes there is an obvious path from django/drf Model to the DRF serializer that should
be used for it's validation. If so, then it may be possible to validation django-admin created objects with the DRF serializer/validators.

https://docs.djangoproject.com/en/3.1/ref/contrib/admin/#admin-custom-validation
https://docs.djangoproject.com/en/3.1/topics/forms/modelforms/#overriding-modelform-clean-method
https://stackoverflow.com/questions/62936732/is-there-a-silver-bullet-for-validation-in-django-rest-framework#comment111299393_62936732

[bmbouter]

re model validation and drf

one option may be to use a custom MyDRFModelForm for the base ModelAdmin class.
MyDRFModelForm would provide a custom .clean() that instead of calling just the django Model.clean()
validation, but could also in theory call the DRF validation.

For the latter, that assumes there is an obvious path from django/drf Model to the DRF serializer that should
be used for it's validation. If so, then it may be possible to validation django-admin created objects with the DRF serializer/validators.

https://docs.djangoproject.com/en/3.1/ref/contrib/admin/#admin-custom-validation
https://docs.djangoproject.com/en/3.1/topics/forms/modelforms/#overriding-modelform-clean-method
https://stackoverflow.com/questions/62936732/is-there-a-silver-bullet-for-validation-in-django-rest-framework#comment111299393_62936732

I will read through this more, but overall if we can get it working with DRF validation this would be hella great.

[bmbouter]

@alikins Pulp at startup for all plugin builds a mapping of model names to corresponding Viewsets here and each viewset concretely declares its serializer. So while it's possible there are some gotchas.

1. It only works with viewsets that are subclasses of NamedModelViewSet. I think this kind of defeats the reasoning I read for this feature to start with which was: "its used in cases where there is no API yet but there is a model". In that situation there is no viewset at all.

2. It won't work for all kinds of Serializers because sometimes they differ signifcantly from the Model. For instance the CollectionSerializer to create a Collection from takes in a tarball, and the model data it produces has nothing of the same fields as the serializer.

If "raw database fixup" is a requirement then I think deploying an app on the platform that provides that feature set would be ideal. Otherwise I think sticking to the API usage is my recommendation. All data in pulp_ansible for example has endpoints for the models that back it, so I can't think of anything you'd want to fix via django-admin that you couldn't fix via the API.

As an aside, we do want to use django-admin for the object-level RBAC, but that is data around the models not the model data itself.

[bmbouter]

The DRF folks on IRC say that we should consider using this https://www.django-rest-framework.org/api-guide/renderers/#adminrenderer

[bmbouter]

After more slack discussion my understanding is that the interest is to actually have it writeable and meanwhile we require it to be read-only. To summarize the two concerns about why writable django-admin is not safe for Pulp users:

1. It bypasses the DRF serializers
2. Pulp's highly relational data isn't easy or safe to modify one model at a time

Since what you'll get isn't actually what you need if galaxy_ng really needs this, here is what I recommend

1. (recommended best solution) If you require raw-db read/write access use a tool built for it like https://www.pgadmin.org/screenshots/
2. (second best recommendation) Use the API for read/write. All models you'd get here you can use through Pulp's API which does flow through the DRF serializer and also handled multi-model relationships correctly.
3. (third best) If you absolutely want to continue with this, reopen this PR against galaxy_ng

For all these reasons I'm closing for now.