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 <[email protected]> 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 <[email protected]> wrote: > >> +1 >> >> David >> >> >> On Tue, Dec 18, 2018 at 9:31 AM Brian Bouterse <[email protected]> >> 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 <[email protected]> >>> 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 <[email protected]> >>>> wrote: >>>> >>>>> >>>>> >>>>> On Tue, Dec 18, 2018 at 9:07 AM Tatiana Tereshchenko < >>>>> [email protected]> 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 <[email protected]> >>>>>> 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 <[email protected]> >>>>>>> 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 < >>>>>>>> [email protected]> 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 <[email protected]> >>>>>>>>> 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 < >>>>>>>>>> [email protected]> 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 >>>>>>>>>>> [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 >>>>>>>> >>>>>>> _______________________________________________ >>>>> 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 >
_______________________________________________ Pulp-dev mailing list [email protected] https://www.redhat.com/mailman/listinfo/pulp-dev
