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 <ttere...@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 <ipan...@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 <amacd...@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 <ttere...@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 <davidda...@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 < >>>>> ttere...@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 < >>>>>> ttere...@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 <dal...@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 <davidda...@redhat.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> +1 >>>>>>>>> >>>>>>>>> David >>>>>>>>> >>>>>>>>> >>>>>>>>> On Tue, Dec 18, 2018 at 9:31 AM Brian Bouterse < >>>>>>>>> bbout...@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 < >>>>>>>>>> davidda...@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 < >>>>>>>>>>> bbout...@redhat.com> wrote: >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Tue, Dec 18, 2018 at 9:07 AM Tatiana Tereshchenko < >>>>>>>>>>>> ttere...@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 < >>>>>>>>>>>>> ipan...@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 < >>>>>>>>>>>>>> bbout...@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 < >>>>>>>>>>>>>>> ttere...@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 < >>>>>>>>>>>>>>>> davidda...@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 < >>>>>>>>>>>>>>>>> ttere...@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@redhat.com >>>>>>>>>>>>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>>>>> Pulp-dev mailing list >>>>>>>>>>>>>>>> Pulp-dev@redhat.com >>>>>>>>>>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>>>> Pulp-dev mailing list >>>>>>>>>>>>>>> Pulp-dev@redhat.com >>>>>>>>>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>>>>>>>>>> >>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>> Pulp-dev mailing list >>>>>>>>>>>> Pulp-dev@redhat.com >>>>>>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>>>>>>> >>>>>>>>>>> _______________________________________________ >>>>>>>>> Pulp-dev mailing list >>>>>>>>> Pulp-dev@redhat.com >>>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> Pulp-dev mailing list >>>>>>>> Pulp-dev@redhat.com >>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>>> >>>>>>> _______________________________________________ >>>> Pulp-dev mailing list >>>> Pulp-dev@redhat.com >>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>> >>> _______________________________________________ >>> Pulp-dev mailing list >>> Pulp-dev@redhat.com >>> https://www.redhat.com/mailman/listinfo/pulp-dev >>> >> _______________________________________________ > Pulp-dev mailing list > Pulp-dev@redhat.com > https://www.redhat.com/mailman/listinfo/pulp-dev >
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev