+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
