[Pulp-dev] Namespacing plugins, looking for feedback

Ina Panova ipanova at redhat.com
Thu Jan 3 12:15:01 UTC 2019 

+1 namespacing all master/detail

For consistency, i would prefer to see same format i see in
`content_summary` as in content endpoints, even if it does not make sense
from user's perspective, because what we now have in content_summary, i
would not say that it makes much sense from user's perspective ;)

--------
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 Wed, Jan 2, 2019 at 8:55 PM Austin Macdonald <amacdona at redhat.com> wrote:

> +1 automatic namespacing for master/detail. I realize the easiest way to
> do this would be to use the app_label, giving us:
>
> /api/v3/content/pulp_rpm/packages/
>
>
> However, I feel like this url is pretty clunky. The "pulp_" is totally
> unnecessary, from the user's perspective. Instead, I think I'd prefer to
> add an attribute to the App config.
>
> https://github.com/pulp/plugin_template/blob/master/pulp_plugin_template/app/__init__.py#L8
>
> `endpoint_namespace = rpm` or `short_label = rpm`
>
> Result: /api/v3/content/rpm/packages/
>
> The downside is that every plugin would need 1 more line of code. The
> upside is that we could implement it exactly same way as app_label but
> without url redundancy.
>
> On Wed, Jan 2, 2019 at 2:39 PM Tatiana Tereshchenko <ttereshc at redhat.com>
> wrote:
>
>> 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
>>>>>>
>>>>> _______________________________________________
>> 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/20190103/83757867/attachment.htm>

More information about the Pulp-dev mailing list