Switch Distribution to Master/Detail

[gmbnomis]

The models offered by pulpcore are:

• BaseDistribution: a (non-abstract) MasterModel. This class can be used in
  plugins that don't use publications.

• Distribution: a subclass of BaseDistribution. This class is the "standard"
  Distributor that will be subclassed by plugins using the standard content
  app.

Required PR: pulp/pulpcore-plugin#95
Required PR: pulp/pulp_file#217
Required PR: pulp/pulp-smash#1205
Required PR: pulp/pulp-certguard#20
Required PR: pulp/pulp-openapi-generator#12

ref #4785
https://pulp.plan.io/issues/4785

[codecov]

Codecov Report

Merging #117 into master will decrease coverage by <.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
- Coverage   67.86%   67.85%   -0.01%     
==========================================
  Files          65       64       -1     
  Lines        3015     2993      -22     
==========================================
- Hits         2046     2031      -15     
+ Misses        969      962       -7

Impacted Files	Coverage Δ
pulpcore/app/viewsets/base.py	93.08% <100%> (+0.51%)	⬆️
pulpcore/app/viewsets/publication.py	100% <100%> (ø)	⬆️
pulpcore/app/serializers/publication.py	97.77% <100%> (-1.12%)	⬇️
pulpcore/app/tasks/__init__.py	100% <100%> (ø)	⬆️
pulpcore/app/serializers/repository.py	100% <100%> (ø)	⬆️
pulpcore/app/models/publication.py	71.23% <100%> (-0.77%)	⬇️
pulpcore/app/viewsets/__init__.py	100% <100%> (ø)	⬆️
pulpcore/app/tasks/base.py	22.72% <22.22%> (-0.35%)	⬇️
pulpcore/app/serializers/task.py	98.07% <0%> (-1.93%)	⬇️
pulpcore/app/serializers/base.py	94.87% <0%> (-1.29%)	⬇️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e706e7a...0fe3cbd. Read the comment docs.

[gmbnomis]

@dkliban @bmbouter This looks good now from my point of view. Do you want to have a look?

[bmbouter]

I'm going to test this out today. Thank you @gmbnomis !

[bmbouter]

In some irc/bluejeans convo prior to this PR we observed these options are mutually exclusive. Design-wise that motivates splitting Distribution into two models, class PublicationDistribution(BaseDistribution) and class RepositoryVersionDistribution(BaseDistribution).

This has two significant positive effects. 1) it prevents say pulp_file users incorrectly specifying Distribution.repository or Distribution.repository_version which won't work because it needs a Publication since there is metadata. 2) it makes the docs much clearer so that the only options shown are those that are intented for use.

[bmbouter]

I also had imagined that core would allow these PublicationDistribution and RepositoryVersionDistribution as-is (that is they specify the TYPE field). This would allow plugin writers to avoid specifying another type. What do you think about this?

[gmbnomis]

Yes, I think we should have these as base classes. However, if plugins would use these types directly, would we have endpoints like(.../distributions/core/publication and .../distributions/core/version?

[gmbnomis]

If so, you would not be able to decide what type of distribution a particular plugin supports from the API specification. If we 'force' the plugin to use a detail viewset (and derived model) it becomes clear what is supported by looking at the fields in the endpoint, e.g. distributions/file/file/

[dkliban]

I don't see any changes in the handler for the pulp-content-app. I expected to see something related to limiting which Distributions are served by the pulp-content-app. Prior to this change the handler provided py pulpcore only looked at Distributions when matching the base_path. Now it needs to check all types of Distributions, but not others like DockerDistribution.

[gmbnomis]

@dkliban

I don't see any changes in the handler for the pulp-content-app. I expected to see something related to limiting which Distributions are served by the pulp-content-app. Prior to this change the handler provided py pulpcore only looked at Distributions when matching the base_path. Now it needs to check all types of Distributions, but not others like DockerDistribution.

The idea is that Docker derives from BaseDistribution and thus, would not be visible in the default content-app. But that is probably to coarse. I think there are three use cases:

1. pulp-content-app is sufficient for plugin
2. plugin needs to provide some additional URLs but artifact content is still served by the pulp-content-app (although not implement this way, pulp_cookbook could fit into this category)
3. plugin needs a different content app, the plugin's content should not be visible in the default app.

We could introduce a content app name field in the distribution. Then, we would have:
case 1. Use the default type, e.g. 'core'
case 2. Use the default type + the additional URLs are served for the specific Distribution subclass
case 3. Define a new type, e.g. 'docker'

base path overlap checks would have to be done per content app name

What do you think? (If we do this, I think we should not do it in this PR, though)

[dkliban]

@gmbnomis The behavior you describe as it is implemented in this PR makes sense to me. I agree that we should consider allowing plugin writers to specify if their Distribution should be visible to the default content app or not

[bmbouter]

I've been using assert for this kind of checking for years and I like it. Recently, I've been told that some environments run their interpreters with assert checking disabled and for that reason this should be avoided. Have you ever heard of someone doing that? Do you think this is something we should be concerned with?

[gmbnomis]

Good point. I am very cautious using assert for this reason. But I think it is ok in this case: It will raise as soon someone uses create() from AsyncCreateMixin without defining a custom async_reserved_resources(). IMHO, if someone uses AsynCreateMixin without writing a test (that will blow up immediately), it's their fault ;-)

But I can change that, of course (there another us of assert in this file already)

[bmbouter]

Yeah I was more just curious. We have this elsewhere so we can leave as-is for now. We can re-approach this later.

[bmbouter]

I read through all this and it looks great! I also tested it all against the pulp_file plugin and everything worked there as expected. I think we're good to merge it since you've also answered all the questions from @dkliban. As discussed on the PR there will be followon work to split the classes into distinct objects anyway.

Thank you! 🥇 👍 🌴 🍊