[Pulp-dev] Namespacing plugins, looking for feedback

Fri Jan 4 19:32:25 UTC 2019

+1 to dropping the "pulp_" I don't think it's a helpful convention.

I also want to avoid adding another plugin configuration attribute. +1 to
automatic namespacing and using the django app label.

On Fri, Jan 4, 2019 at 2:18 PM David Davis <daviddavis at redhat.com> wrote:

> While I am not opposed to dropping the "pulp_" prefix in URLs, I don't
> like the idea of adding another attribute to the plugin configuration.
>
> I am +1 to automatic namespacing and using the django app label.
>
> David
>
>
> On Fri, Jan 4, 2019 at 1:50 PM Tatiana Tereshchenko <ttereshc at redhat.com>
> wrote:
>
>> It's seems that there is a consensus that all master/detail related
>> endpoints should be prepended.
>> There is no consensus if the current Django app label is good enough to
>> use in the construction of the endpoints.
>>
>> My personal opinion:
>> It's probably better if "pulp_" part goes away, at the same time I'm
>> hesitant to add new attributes to configuration, since Django provides
>> enough of them and gives us uniqueness check for free.
>> See Django docs
>> https://docs.djangoproject.com/en/2.1/ref/applications/#configurable-attributes
>> Label is supposed to be a short name for the app.
>>
>> Please vote and/or raise your concerns if you have any by next Friday,
>> voting will be closed on Jan 11, 2019 at 8:00PM EST.
>> The proposal is described in https://pulp.plan.io/issues/4279
>>
>> My vote:
>> +1 for automatic namespacing for all master/detail endpoints
>> +1 to use existing Django app label
>>
>> Thank you,
>> Tanya
>>
>> On Thu, Jan 3, 2019 at 1:15 PM Ina Panova <ipanova at redhat.com> wrote:
>>
>>> +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
>>>>
>>> _______________________________________________
>> 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/20190104/3a326043/attachment.htm>