FYI, I merged https://github.com/pulp/pulpcore/pull/319. It was sort of a breaking change but I hope no one was relying on the broken behavior[0].
[0] https://imgs.xkcd.com/comics/workflow.png David On Thu, Oct 3, 2019 at 9:07 AM Tatiana Tereshchenko <ttere...@redhat.com> wrote: > 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 <ttere...@redhat.com> > 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 <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
