Pulp

Would it be possible to name the methods you want to move here and from/to where?

If not already planned anyway, I suggest to include the distribution_model attribute from the Docker content app. I think it is useful to be able to select the distribution class to use. I would prefer giving the default on class level though (like e.g. serializer_class in the viewsets)

gmbnomis wrote:

If not already planned anyway, I suggest to include the distribution_model attribute from the Docker content app. I think it is useful to be able to select the distribution class to use. I would prefer giving the default on class level though (like e.g. serializer_class in the viewsets)

I agreed with this and went to add it in to the new `BaseHandler` but I realized that since master/detail is now here maybe we don't need distribution_model at all since the `BaseDistribution(....).cast()` will automatically give you the correct detail type. Or is the idea that we want to query on the detail type directly to restrict what that subclass handler will match?

Also should we move the _match_distribution and it's supporting function _base_path to `BaseHandler`?

So far I've posted my changed, but I think we probably additional reworking of what is offered from `BaseHandler`.

bmbouter wrote:

gmbnomis wrote:
I agreed with this and went to add it in to the new `BaseHandler` but I realized that since master/detail is now here maybe we don't need distribution_model at all since the `BaseDistribution(....).cast()` will automatically give you the correct detail type. Or is the idea that we want to query on the detail type directly to restrict what that subclass handler will match?

Yes, that's the idea. I see it as the 'other half' of #4809. A plugin may want to serve it's distributions at a non-default path (pulp_cookbook is an example of that). Of course we could search all distributions first, cast the result and return a 404 if it is not the right type. But all that is not needed as we can restrict the query to the correct type from the beginning.

Also should we move the _match_distribution and it's supporting function _base_path to `BaseHandler`?

So far I've posted my changed, but I think we probably additional reworking of what is offered from `BaseHandler`.

For pulp_cookbook, the current `BaseHandler` looks like a huge step backward. pulp_cookbook's content handler basically takes a very slightly adapted default handler and adds a live URL on top (see https://github.com/gmbnomis/pulp_cookbook/blob/345ce3a911905609238c65b8a92bbf4e0f125552/pulp_cookbook/app/content/handler.py#L19).

Now, that that default handler is not part of the plugin API anymore, I will have to copy lots of code.

@gmnomis w.r.t the querying I'm + 1 to adding DISTRIBUTION_MODEL and having the matching machinery use that versus casting it to it's detail.

I can see how splitting this into two objects is creating problems and I see benefit to keeping it together and having all Handlers subclass the single one provided by core (how it was before my PRs).

That begs the question for pulp_docker, can your Registry build off of Handler and if not, why not? If it could then we would close these PRs.

One thing I can see making subclassing of Handler hard is that the use of staticmethods causes the hardcoded class name of Handler to prevent additional customization of subclasses. We can fix that easily by switching those to classmethods.

I think I should be able to use the whole handler in pulp_docker by overriding '_match_distribution' and not using the _match_and_stream method.

bmbouter wrote:

One thing I can see making subclassing of Handler hard is that the use of staticmethods causes the hardcoded class name of Handler to prevent additional customization of subclasses. We can fix that easily by switching those to classmethods.

I am ambivalent on this question (in a broader sense than your question on static methods): The Handler class has considerable size and complexity now and it is not clear which parts of it are implementation details and which parts are open to customization by subclassing.

To make things even more complex, Handler seems to provide a mixture of two things:
- Reusable code that may come handy in content apps ('library API' style, e.g. methods that stream content)
- Hooks to customize behavior (plugin-API style, e.g. a plugin may want to do additional things on _save_content)

I don't know whether we should do this right now, but I would like to see those two types of reuse separated and documented. If we don't do this, I fear that almost any change in Handler might break some plugin. Does this make sense?

@dkliban cool so it sounds like I can close these PRs and pulp_docker should port onto the existing `Handler` in the API (at least for the immediate plan). I also need to make a new PR that will:

• make the staticmethods cls methods to allow customization for subclasses
• move the HOP_BY_HOP to Handler because it's not in the plugin API currently

gmbnomis, I agree w/ your observation and concerns. I believe what we should do is think about which of these methods are expected to be overridden and which aren't and make the overridable ones public and the non-overridable ones (internals) private.

From @dkliban's post it sounds like '_match_distribution' should become public. What else needs to be public? What should be private that is already public?