[Pulp-dev] Namespacing plugins, looking for feedback

Daniel Alley dalley at redhat.com
Wed Dec 19 17:17:36 UTC 2018 

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 <daviddavis at redhat.com> wrote:

> +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
>>>>
>>> _______________________________________________
> 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/20181219/c18d745f/attachment.htm>

More information about the Pulp-dev mailing list