[Pulp-dev] Namespacing plugins, looking for feedback

Tue Dec 18 14:38:09 UTC 2018

+1

David

On Tue, Dec 18, 2018 at 9:31 AM Brian Bouterse <bbouters at 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 <daviddavis at 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 <bbouters at redhat.com>
>> wrote:
>>
>>>
>>>
>>> On Tue, Dec 18, 2018 at 9:07 AM Tatiana Tereshchenko <
>>> ttereshc at 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 <ipanova at 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 <bbouters at 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 <
>>>>>> ttereshc at 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 <daviddavis at 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 <
>>>>>>>> ttereshc at 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 at redhat.com
>>>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>> Pulp-dev mailing list
>>>>>>> Pulp-dev at redhat.com
>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>>>
>>>>>> _______________________________________________
>>>>>> Pulp-dev mailing list
>>>>>> Pulp-dev at redhat.com
>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>>
>>>>> _______________________________________________
>>> Pulp-dev mailing list
>>> Pulp-dev at redhat.com
>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20181218/12c31db5/attachment.htm>