Thank you all for the discussion so far. The question - the type field and namespacing in content summary - is solved with https://pulp.plan.io/issues/4185.
The last remaining question is whether we want to prepend endpoints for master/detail models with plugin label. If yes, then everything or for Content only. See details on the issue https://pulp.plan.io/issues/4279. Examples of the suggested change: /api/v3/content/rpm/packages/ --> /api/v3/content/pulp_rpm/packages/ /api/v3/remotes/rpm/ --> /api/v3/content/remotes/pulp_rpm/rpm/ /api/v3/publishers/rpm/ --> /api/v3/content/publishers/pulp_rpm/rpm/ Changes which will be needed in plugins: - adjust the value of the `endpoint_name` attribute in the viewsets we introduce changes to Please provide feedback, here or on the issue https://pulp.plan.io/issues/4279. This is an RC blocker, so it would be great to groom it over the next couple of days. Thank you, Tanya On Thu, Dec 20, 2018 at 9:41 AM Tatiana Tereshchenko <ttere...@redhat.com> wrote: > Since we are leaning towards prepending types for _all_ master/detail > models and not only for the content model, that Django fix is no longer > important for us. > > Tanya > > On Wed, Dec 19, 2018 at 6:18 PM Daniel Alley <dal...@redhat.com> wrote: > >> David, was that a vote to make it explicit? >> >> I would regard this as fairly intuitive as far as "magic-ness" goes, >> acceptable from the user POV in my opinion. And if Django is explicitly >> trying to support this functionality and relies on it working properly, and >> has a unittest for it going forwards, then I'm fairly confident it won't be >> too fragile. My vote is +1. >> >> My only concern (and it's not a major one) is that a plugin that needed >> to be renamed might have problems with this. But I think that would be >> resolvable with a migration. >> >> Tanya, will we need to remove the workaround once Django 2.2 comes out? >> If so, we should file a Refactor task for it. >> >> On Tue, Dec 18, 2018 at 9:39 AM David Davis <davidda...@redhat.com> >> wrote: >> >>> +1 >>> >>> David >>> >>> >>> On Tue, Dec 18, 2018 at 9:31 AM Brian Bouterse <bbout...@redhat.com> >>> wrote: >>> >>>> There is also an issue w/ my suggestion in that it's highly magical. >>>> The class name is likely going to go through a case mutation and if not >>>> it's going to be finicky in terms of its case. So now I'm thinking the >>>> plugin writer should have to define it to keep it simple and explicit (not >>>> implicit and magical). >>>> >>>> On Tue, Dec 18, 2018 at 9:27 AM David Davis <davidda...@redhat.com> >>>> wrote: >>>> >>>>> Would it be possible to default to class name but let plugin writers >>>>> override this? I would imagine in some cases plugin writers might want to >>>>> change the name (eg cases where you can't use type as the class name like >>>>> File). >>>>> >>>>> David >>>>> >>>>> >>>>> On Tue, Dec 18, 2018 at 9:23 AM Brian Bouterse <bbout...@redhat.com> >>>>> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Tue, Dec 18, 2018 at 9:07 AM Tatiana Tereshchenko < >>>>>> ttere...@redhat.com> wrote: >>>>>> >>>>>>> Brian, >>>>>>> the current PR [0] does exactly what you describe, it uses label >>>>>>> which is taken from the plugin subclass of PulpPluginAppconfig + TYPE >>>>>>> defined on the detail model. >>>>>>> FWIW, there is an option to use plugin class name and not a plugin >>>>>>> writer defined TYPE, e.g. pulp_file.filecontent, pulp_rpm.package, >>>>>>> pulp_rpm.updaterecord, etc. >>>>>>> >>>>>> +1 to using the classname. Having the plugin class name used would >>>>>> allow the user to not repeat themselves as much. I think it's likely the >>>>>> class name == TYPE in almost all cases. The plugin writer would have 1 >>>>>> less >>>>>> requirement on them at Content model definition time and that helps >>>>>> achieve >>>>>> the "less burden on plugin writers" goal for pulp. >>>>>> >>>>>>> >>>>>>> Jeff, to answer your questions: >>>>>>> >>>>>>>> 1. why do all the plugins begin with "pulp_"? >>>>>>>> >>>>>>> This is how django app label is defined in every plugin so far, see >>>>>>> pulp_file case [1]. >>>>>>> Whatever is defined there is used as a plugin name. >>>>>>> >>>>>>> 2. can the plugin name get pre-pended when it's loaded by core? >>>>>>>> >>>>>>>> I lean toward TYPE=<plugin>.<type> >>>>>>> >>>>>>> Just to clarify, there is a class arttriburte `TYPE` and there is a >>>>>>> `type` field on a model. I guess you suggest type = <plugin>.<TYPE>. >>>>>>> >>>>>>> We can probably do it on a master model in the save method [2], just >>>>>>> initially the change was proposed for Content models only. >>>>>>> If we decide to namespace all master/detail objects, I agree we can >>>>>>> do it n a more generic way, than just redefine __init__ on a specific >>>>>>> class. >>>>>>> >>>>>> >>>>>>> Tanya >>>>>>> >>>>>>> [0] https://github.com/pulp/pulp/pull/3801 >>>>>>> [1] >>>>>>> https://github.com/pulp/pulp_file/blob/24881314372b9c1c505ff687c15238126b261afa/pulp_file/app/__init__.py#L10 >>>>>>> [2] >>>>>>> https://github.com/pulp/pulp/blob/master/pulpcore/app/models/base.py#L76-L83 >>>>>>> >>>>>>> On Tue, Dec 18, 2018 at 12:58 PM Ina Panova <ipan...@redhat.com> >>>>>>> wrote: >>>>>>> >>>>>>>> +1 to namespace master/detail as well. >>>>>>>> +1 to Brian's suggestion to try. >>>>>>>> >>>>>>>> -------- >>>>>>>> Regards, >>>>>>>> >>>>>>>> Ina Panova >>>>>>>> Software Engineer| Pulp| Red Hat Inc. >>>>>>>> >>>>>>>> "Do not go where the path may lead, >>>>>>>> go instead where there is no path and leave a trail." >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Dec 18, 2018 at 12:15 AM Brian Bouterse < >>>>>>>> bbout...@redhat.com> wrote: >>>>>>>> >>>>>>>>> +1 to namespacing all Master/Detail objects (Remotes, Publishers, >>>>>>>>> etc). Namespacing will increase consistency w/ the user experience >>>>>>>>> and will >>>>>>>>> avoid plugin-to-plugin naming collisions. @ttereshc +1 to the url >>>>>>>>> changes >>>>>>>>> and content summary changes you've described. >>>>>>>>> >>>>>>>>> I think it would be ideal if the app specified its 'label' >>>>>>>>> attribute on the PulpPluginAppconfig subclass, e.g here in pulp_file >>>>>>>>> https://github.com/pulp/pulp_file/blob/24881314372b9c1c505ff687c15238126b261afa/pulp_file/app/__init__.py#L10 >>>>>>>>> Then the Model for, e.g. the FileContent would have the second >>>>>>>>> portion of >>>>>>>>> the string 'file' as an example and Master/Detail would assemble them. >>>>>>>>> >>>>>>>>> Is this implementation how you imagined it? >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Mon, Dec 17, 2018 at 9:29 AM Tatiana Tereshchenko < >>>>>>>>> ttere...@redhat.com> wrote: >>>>>>>>> >>>>>>>>>> Just to clarify, the type field is not used in the endpoint >>>>>>>>>> construction, so two changes described in the original e-mail are >>>>>>>>>> independent. >>>>>>>>>> >>>>>>>>>> In my opinion: >>>>>>>>>> - it is possible to have type collisions. >>>>>>>>>> - it is possible to have the same endpoints (endpoint_name in a >>>>>>>>>> viewset). >>>>>>>>>> >>>>>>>>>> FWIW, the endpoint collision is not unique to the master/detail >>>>>>>>>> models' endpoints. A plugin, in theory, can define any endpoint they >>>>>>>>>> want. >>>>>>>>>> Though not preventing collisions it for endpoints related to >>>>>>>>>> master/detail models makes it easier to create such collision >>>>>>>>>> accidentally. >>>>>>>>>> >>>>>>>>>> Tanya >>>>>>>>>> >>>>>>>>>> On Mon, Dec 17, 2018 at 2:27 PM David Davis < >>>>>>>>>> davidda...@redhat.com> wrote: >>>>>>>>>> >>>>>>>>>>> Is it possible (under the current model, without namespacing) to >>>>>>>>>>> have type collisions in the database for master/detail models? Like >>>>>>>>>>> what if >>>>>>>>>>> two plugins define two Contents with the same type or two Remotes >>>>>>>>>>> with the >>>>>>>>>>> same type? This kind of leads me to believe we should namespace >>>>>>>>>>> everything. >>>>>>>>>>> On the Ansible plugin for example, I started working on a git >>>>>>>>>>> Remote[0]. >>>>>>>>>>> Luckily I chose "ansible_git" as the type but I could see plugin >>>>>>>>>>> writers >>>>>>>>>>> running into problems if they are not so careful. >>>>>>>>>>> >>>>>>>>>>> [0] >>>>>>>>>>> https://github.com/pulp/pulp_ansible/pull/38/files#diff-debb42c875c19140793de39be3696ee3 >>>>>>>>>>> >>>>>>>>>>> David >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Sun, Dec 16, 2018 at 4:41 PM Tatiana Tereshchenko < >>>>>>>>>>> ttere...@redhat.com> wrote: >>>>>>>>>>> >>>>>>>>>>>> There is an issue [0] of colliding type names in the content >>>>>>>>>>>> summary which evolved into more general namespacing problem for >>>>>>>>>>>> plugins. >>>>>>>>>>>> >>>>>>>>>>>> The suggested changes [1] are: >>>>>>>>>>>> 1. include plugin name into the content summary >>>>>>>>>>>> >>>>>>>>>>>> "content_summary": { >>>>>>>>>>>> "pulp_rpm.package": 50, >>>>>>>>>>>> "pulp_rpm.errata": 2, >>>>>>>>>>>> "pulp_file.file": 5 >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> 2. include plugin name into content endpoints >>>>>>>>>>>> /api/v3/content/file/files/ --> /api/v3/content/pulp_file/files/ >>>>>>>>>>>> /api/v3/content/rpm/packages/ --> >>>>>>>>>>>> /api/v3/content/pulp_rpm/packages/ >>>>>>>>>>>> /api/v3/content/rpm/errata/ --> /api/v3/content/pulp_rpm/errata/ >>>>>>>>>>>> ... >>>>>>>>>>>> >>>>>>>>>>>> For the change #1, not only content summary output is changed >>>>>>>>>>>> but the type itself in the database. If the content type is used >>>>>>>>>>>> somewhere >>>>>>>>>>>> in the filters, it should be specified in that format: >>>>>>>>>>>> "plugin_name.plugin_type". Does it makes sense to extend the >>>>>>>>>>>> master model >>>>>>>>>>>> and have a plugin name field and a type field, instead of putting >>>>>>>>>>>> preformatted string into the type field? >>>>>>>>>>>> >>>>>>>>>>>> For the change #2, endpoints are namespaced only for the >>>>>>>>>>>> content endpoint and not for other endpoints related to >>>>>>>>>>>> master/detail >>>>>>>>>>>> models, like remotes, publishers, etc. It's inconsistent, however >>>>>>>>>>>> it makes >>>>>>>>>>>> the most sense to have it for content endpoints. >>>>>>>>>>>> >>>>>>>>>>>> Any concerns or thoughts? >>>>>>>>>>>> >>>>>>>>>>>> Thank you, >>>>>>>>>>>> Tanya >>>>>>>>>>>> >>>>>>>>>>>> [0] https://pulp.plan.io/issues/4185#note-8 >>>>>>>>>>>> [1] https://github.com/pulp/pulp/pull/3801 >>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>> 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 >>>>>>>>> >>>>>>>> _______________________________________________ >>>>>> 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 >> >
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
