On 1/4/19 1:32 PM, Brian Bouterse wrote:
> +1 to dropping the "pulp_" I don't think it's a helpful convention.
Couldn't agree more.
>
> 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
> <mailto: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 <mailto: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
> <mailto: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 <mailto: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 <mailto: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
> <mailto: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
> <mailto: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
> <mailto: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
> <mailto: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
> <mailto:daviddavis at redhat.com>> wrote:
>
> +1
>
> David
>
>
> On Tue, Dec 18, 2018 at 9:31
> AM Brian Bouterse
> <bbouters at redhat.com
> <mailto: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
> <mailto: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
> <mailto:bbouters at redhat.com>>
> wrote:
>
>
>
> On Tue, Dec 18,
> 2018 at 9:07 AM
> Tatiana
> Tereshchenko
> <ttereshc at redhat.com
> <mailto: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
> <mailto: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
> <mailto: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
> <mailto: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
> <mailto: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
> <mailto: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
> <mailto:Pulp-dev at redhat.com>
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
> _______________________________________________
> Pulp-dev
> mailing
> list
> Pulp-dev at redhat.com
> <mailto:Pulp-dev at redhat.com>
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
> _______________________________________________
> Pulp-dev
> mailing
> list
> Pulp-dev at redhat.com
> <mailto:Pulp-dev at redhat.com>
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
> _______________________________________________
> Pulp-dev mailing list
> Pulp-dev at redhat.com
> <mailto:Pulp-dev at redhat.com>
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
> _______________________________________________
> Pulp-dev mailing list
> Pulp-dev at redhat.com
> <mailto:Pulp-dev at redhat.com>
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
> _______________________________________________
> Pulp-dev mailing list
> Pulp-dev at redhat.com
> <mailto:Pulp-dev at redhat.com>
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
> _______________________________________________
> Pulp-dev mailing list
> Pulp-dev at redhat.com <mailto:Pulp-dev at redhat.com>
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
> _______________________________________________
> Pulp-dev mailing list
> Pulp-dev at redhat.com <mailto:Pulp-dev at redhat.com>
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
> _______________________________________________
> Pulp-dev mailing list
> Pulp-dev at redhat.com <mailto:Pulp-dev at redhat.com>
> https://www.redhat.com/mailman/listinfo/pulp-dev
>
> _______________________________________________
> Pulp-dev mailing list
> Pulp-dev at redhat.com <mailto: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/20190108/2db7f609/attachment.htm>