Thank you! I filed a task to provide a read-only content viewset https://pulp.plan.io/issues/5535.
Tanya On Wed, Oct 2, 2019 at 8:38 PM David Davis <davidda...@redhat.com> wrote: > I filed an issue[0] and opened a couple PRs which demonstrate how to fix > the problem: > > https://github.com/pulp/pulpcore/pull/319 > https://github.com/pulp/pulp_rpm/pull/1465 > > [0] https://pulp.plan.io/issues/5533 > > David > > > On Wed, Oct 2, 2019 at 1:32 PM Tatiana Tereshchenko <ttere...@redhat.com> > wrote: > >> +1 to what David said. Thanks for detailed explanation. >> >> +1 to file an issue to make it possible to define a custom endpoint for a >> Detail model and keep automatic prepending at the same time. >> >> >> On Wed, Oct 2, 2019 at 7:06 PM David Davis <davidda...@redhat.com> wrote: >> >>> No, it's more complicated than that. There's a bit of magic for >>> Master/Detail model endpoints[0] where the NamedModelViewSet tries to form >>> the endpoint by looking up the master_endpoint, app_label, and >>> endpoint_name (and combining them). In the case of distribution_trees, >>> there is no master viewset so the master_endpoint becomes >>> 'distribution_trees' and thus the url we end up with is >>> /pulp/api/v3/distribution_trees/rpm/distribution_trees/. If we used >>> 'content', it would just be /pulp/api/v3/distribution_trees/rpm/content/. >>> >>> This is a problem we should probably fix regardless of how we handle >>> Tanya's problem. To restate the problem: it's impossible (or very hard) to >>> create your own endpoint for a Detail model because the code automatically >>> tries to namespace your endpoint for you. I can file an issue if others >>> agree. >>> >>> [0] >>> https://github.com/pulp/pulpcore/blob/5bd0f31604de3079c8bb1b710155d7c6e94d7425/pulpcore/app/viewsets/base.py#L185-L216 >>> >>> David >>> >>> >>> On Wed, Oct 2, 2019 at 12:52 PM Brian Bouterse <bmbou...@redhat.com> >>> wrote: >>> >>>> >>>> >>>> On Wed, Oct 2, 2019 at 12:44 PM David Davis <davidda...@redhat.com> >>>> wrote: >>>> >>>>> >>>>> On Wed, Oct 2, 2019 at 12:37 PM Brian Bouterse <bmbou...@redhat.com> >>>>> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Wed, Oct 2, 2019 at 11:03 AM Tatiana Tereshchenko < >>>>>> ttere...@redhat.com> wrote: >>>>>> >>>>>>> Thank you! Good idea to have an additional one. >>>>>>> >>>>>> I agree >>>>>> >>>>>> I'm on the fence between ReadOnly and just Generic one without any >>>>>>> mixins. >>>>>>> >>>>>> I see having the ReadOnlyContentViewset as overall being slightly >>>>>> better than a Generic option for plugin writers for two reasons. >>>>>> >>>>>> 1) the majority of cases (if not all) will want both list and get() >>>>>> mixins so we're handling the common case with this. >>>>>> 2) In the event a plugin writer needs to not have list and/or get(), >>>>>> they could could use NamedModelViewset directly still. Is this accurate? >>>>>> >>>>> >>>>> Per Tanya's original email it sounds like it was hard/impossible to >>>>> roll a viewset at /pulp/api/v3/content/rpm/distribution_trees/. >>>>> >>>> >>>> Oh I see, but isn't that an outcome of `endpoint_name = ' >>>> distribution_trees'` here >>>> <https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/viewsets.py#L256>? >>>> What happens if that becomes `endpoint_name = 'content'`. I believe >>>> the NamedModelViewset would translate that into the url >>>> /pulp/api/v3/content/rpm/distribution_trees/ where the `distribution_trees` >>>> part comes fro the classname. I haven't looking into it in a while though, >>>> so let me know if I'm still not getting it right. >>>> >>>> In the event it's not easy for the plugin writer to be in control of >>>> that then I would be in favor of GenericContentViewset. >>>> >>>> >>>>> >>>>>> >>>>>> Just to write it out for clarity I imagine it would be: >>>>>> ReadOnlyContentViewset(NamedModelViewset, mixins.RetrieveModelMixin, >>>>>> mixins.ListModelMixin) >>>>>> >>>>>> What do you think? >>>>>> >>>>>> >>>>>>> Any other opinions/suggestions? >>>>>>> >>>>>>> On Wed, Oct 2, 2019 at 4:35 PM Matthias Dellweg <dell...@atix.de> >>>>>>> wrote: >>>>>>> >>>>>>>> I would do a variation of 1. : >>>>>>>> Provide a ReadonlyContentViewSet with only GET mixed in and leave >>>>>>>> the >>>>>>>> 'standard' ContentViewset as is. >>>>>>>> Cheers, Matthias >>>>>>>> >>>>>>>> On Wed, 2 Oct 2019 16:16:14 +0200 >>>>>>>> Tatiana Tereshchenko <ttere...@redhat.com> wrote: >>>>>>>> >>>>>>>> > Current implementation of ContentViewset >>>>>>>> > < >>>>>>>> https://github.com/pulp/pulpcore/blob/master/pulpcore/app/viewsets/content.py#L98-L102 >>>>>>>> > >>>>>>>> > includes >>>>>>>> > mixins for create (POST) and retrieve/list (GET). >>>>>>>> > In case a plugin doesn't need to support POST for a content >>>>>>>> endpoint, >>>>>>>> > a plugin writer compiles a viewset from the mixins they need, e.g. >>>>>>>> > distribution trees and custom metadata >>>>>>>> > < >>>>>>>> https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/viewsets.py#L233-L258 >>>>>>>> > >>>>>>>> > (same >>>>>>>> > use case is expected for modularity endpoints) >>>>>>>> > This leads to the inconsistent REST API. >>>>>>>> > >>>>>>>> > # ContentViewset is used >>>>>>>> > /pulp/api/v3/content/rpm/advisories/ >>>>>>>> > >>>>>>>> > # custom plugin content viewset >>>>>>>> > /pulp/api/v3/distribution_trees/rpm/distribution_trees/ >>>>>>>> > >>>>>>>> > Possible solutions: >>>>>>>> > 1. Make ContentViewset more generic (no mixins, or only GET ones?) >>>>>>>> > and let plugins include any mixins they need. >>>>>>>> > This option might be painful to switch to for plugin writers, >>>>>>>> because >>>>>>>> > every plugin will be affected and will need to make this change. >>>>>>>> > At the same time probably not many plugins support upload for >>>>>>>> every >>>>>>>> > content type, so in many cases the POST is broken/not used anyway. >>>>>>>> > >>>>>>>> > 2. Disable POST at the plugin level in some other way. >>>>>>>> > I'm not sure if there is any native option to disable it. >>>>>>>> > Hacky way is to override `create` method which will return >>>>>>>> > appropriate HTTP error that POST is not supported. >>>>>>>> > >>>>>>>> > 3. Make plugin writers manually define a proper endpoint name. >>>>>>>> > Apart from not being reliable, I'm not sure how to do it because >>>>>>>> of >>>>>>>> > how we tweak endpoint generation. >>>>>>>> > Notice the distribution trees example ^, "distribution_trees" is >>>>>>>> used >>>>>>>> > twice in the endpoint. >>>>>>>> > >>>>>>>> > 4. Any other solutions? Easy ones which I missed? >>>>>>>> > >>>>>>>> > Thank you, >>>>>>>> > Tanya >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> Pulp-dev mailing list >>>>>>> Pulp-dev@redhat.com >>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>> >>>>>> _______________________________________________ >>>>>> Pulp-dev mailing list >>>>>> Pulp-dev@redhat.com >>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>> >>>>>
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
