+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