+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 <davidda...@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 <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 >
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
