Thanks everyone for discussion! PRs are up https://github.com/pulp/pulpcore/pull/322 https://github.com/pulp/pulpcore-plugin/pull/140 https://github.com/pulp/pulp_rpm/pull/1467
On Thu, Oct 3, 2019 at 10:27 AM Tatiana Tereshchenko <[email protected]> wrote: > 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 <[email protected]> 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 <[email protected]> >> 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 <[email protected]> >>> 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 <[email protected]> >>>> wrote: >>>> >>>>> >>>>> >>>>> On Wed, Oct 2, 2019 at 12:44 PM David Davis <[email protected]> >>>>> wrote: >>>>> >>>>>> >>>>>> On Wed, Oct 2, 2019 at 12:37 PM Brian Bouterse <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Wed, Oct 2, 2019 at 11:03 AM Tatiana Tereshchenko < >>>>>>> [email protected]> 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 <[email protected]> >>>>>>>> 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 <[email protected]> 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 >>>>>>>> [email protected] >>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> Pulp-dev mailing list >>>>>>> [email protected] >>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>> >>>>>>
_______________________________________________ Pulp-dev mailing list [email protected] https://www.redhat.com/mailman/listinfo/pulp-dev
