[Pulp-dev] Namespacing plugins, looking for feedback

Tue Dec 18 14:30:44 UTC 2018

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/af51e283/attachment.htm>