With regards to the change #1, it seems there are no objections and the +1s are for prepending types for all master/detail models. The PR [0] is updated and awaiting review.
The change #2 (plugin name in endpoints) was not discussed properly here and at the same time it doesn't block the committing-migrations story, so it got moved to a separate story [1], please leave your feedback there. It's still an RC blocker, just not blocking other blockers :) Thank you for your feedback, Tanya [0] https://github.com/pulp/pulp/pull/3801 [1] https://pulp.plan.io/issues/4279 On Tue, Dec 18, 2018 at 3:38 PM 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
