[Pulp-dev] Namespacing plugins, looking for feedback

Tatiana Tereshchenko ttereshc at redhat.com
Wed Jan 2 19:38:13 UTC 2019 

It would be automatic, and plugins need a change only to avoid redundant
prepending.
E.g. If RPM plugin makes no changes, the endpoint for RPM content will be:

/api/v3/content/pulp_rpm/rpm/packages/

because endpoint_name = 'rpm/packages'.

So plugin should leave only endpoint_name = 'packages'.

The endpoint with redundant plugin name will work fine, just doesn't
look good :)

Tanya


On Wed, Jan 2, 2019 at 7:20 PM David Davis <daviddavis at redhat.com> wrote:

> I am +1 to namespacing all master detail models with the plugin name.
> Would this be automatic or something that the plugin writers would be
> encouraged to do?
>
> David
>
>
> On Wed, Jan 2, 2019 at 12:58 PM Tatiana Tereshchenko <ttereshc at redhat.com>
> wrote:
>
>> Thank you all for the discussion so far.
>> The question - the type field and namespacing in content summary - is
>> solved with https://pulp.plan.io/issues/4185.
>>
>> The last remaining question is whether we want to prepend endpoints for
>> master/detail models with plugin label. If yes, then everything or for
>> Content only.
>> See details on the issue https://pulp.plan.io/issues/4279.
>>
>> Examples of the suggested change:
>>
>> /api/v3/content/rpm/packages/ --> /api/v3/content/pulp_rpm/packages/
>> /api/v3/remotes/rpm/ --> /api/v3/content/remotes/pulp_rpm/rpm/
>> /api/v3/publishers/rpm/ --> /api/v3/content/publishers/pulp_rpm/rpm/
>>
>> Changes which will be needed in plugins:
>>   - adjust the value of the `endpoint_name` attribute in the viewsets we introduce changes to
>>
>> Please provide feedback, here or on the issue
>> https://pulp.plan.io/issues/4279.
>> This is an RC blocker, so it would be great to groom it over the next
>> couple of days.
>>
>> Thank you,
>> Tanya
>>
>> On Thu, Dec 20, 2018 at 9:41 AM Tatiana Tereshchenko <ttereshc at redhat.com>
>> wrote:
>>
>>> 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 <dalley at redhat.com> 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 <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
>>>>>
>>>> _______________________________________________
>>>> 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/20190102/bceb6787/attachment.htm>

More information about the Pulp-dev mailing list