Split Distribution into two objects #124

bmbouter · 2019-05-09T13:21:24Z

The Distribution contained options which should not be mixed.
Specifically the repository and repository_version options go together
and the publication goes by itself.

This PR splits that object into two new objects
RepositoryVersionDistribution and PublicationDistribution. The two
models are not detail objects, and require the plugin writer to declare
a Distribution detail object. It was already this way before this PR.

Required PR: pulp/pulpcore-plugin#97
Required PR: pulp/pulp_file#219

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

The Distribution object was split into two objects for concrete use by users. The models are available for plugin writers to reference, but the serializers and viewsets are designed to always be provided by core so they are not available through the plugin API. Required PR: pulp/pulpcore#124 https://pulp.plan.io/issues/4785 closes #4785

pulpcore/app/viewsets/publication.py

Model Changes The Distribution model was replaced with the RepositoryVersionDistribution and PublicationDistribution models. These are both abstract models and are meant to be subclassed further. Serializer Changes The DistributionSerializer is removed and now BaseDistributionSerializer, PublicationDistributionSerializer, and RepositoryVersionDistributionSerializer are here. These can also be further subclassed by plugin writers. ViewSet Changes DistributionViewset is now called BaseDistributionViewSet. No other viewsets are provided. Required PR: pulp/pulpcore#124 Required PR: pulp/pulp_file#219 https://pulp.plan.io/issues/4785 closes #4785

bmbouter · 2019-05-09T19:15:18Z

pulpcore/content/handler.py


        Raises:
            PathNotResolved: when not matched.
        """
        base_paths = Handler._base_paths(path)
        try:
-            return Distribution.objects.get(base_path__in=base_paths)
+            return BaseDistribution.objects.get(base_path__in=base_paths)


This seems problematic to me. This would include custom distributions that are also subclasses of BaseDistribution. Isn't that an issue?

I think BaseDistribution should provide a property called 'served_by_pulpcore' and default it to True.

ok so we know we don't love this, but I think I'm going to file it as a follow story since that's an additive change and bugfix. Sound ok?

I filed the issue here, and I also commented on why maybe it won't happen? https://pulp.plan.io/issues/4809#note-1

gmbnomis · 2019-05-09T18:57:44Z

pulpcore/content/handler.py


        Raises:
            PathNotResolved: when not matched.
        """
        base_paths = Handler._base_paths(path)
        try:
-            return Distribution.objects.get(base_path__in=base_paths)
+            return BaseDistribution.objects.get(base_path__in=base_paths)


This make @dkliban's commtent (#117 (comment)) apply again. However, I think we need a better mechanism than subclassing to control which distributions are served by content apps. Should we leave it as it is for the time being and explore possible refinements later?

I pushed some refinements to the handler code (needed updates) but I also filed this for us to maybe do better in the future in this area: https://pulp.plan.io/issues/4809

gmbnomis · 2019-05-09T19:14:14Z

pulpcore/app/serializers/publication.py

-
-        if len(in_use_keys) > 1:
-            msg = _("The attributes {keys} must be used exclusively.".format(keys=in_use_keys))
+        if 'repository' in data and 'repository_version' in data:


Do you know what happens if those parameters are set to null? I don't know if they disappear or result in None. If the latter, it is better to use data.get('repository') and data.get('repository_version') here

They disappear when I was testing it I was testing with one or the other. I'm also going to write some test issues which will add a functional test for this aspect to give us confidence over time.

gmbnomis · 2019-05-09T19:20:01Z

pulpcore/app/viewsets/publication.py

@@ -95,12 +99,12 @@ def async_reserved_resources(self, instance):
    @classmethod
    def is_master_viewset(cls):


I think this method is not needed anymore. This Viewset now has an associated Model that actually is a master model.

I agree let me remove it now.

yup it works the same without this.

bmbouter · 2019-05-09T19:23:24Z

pulpcore/app/models/publication.py

+
+    class Meta:
+        abstract = True
+        default_related_name = '_distributions'


I don't think L#247 and L#255 can be the same? It's working for me, but that's because there is exactly 1 concrete class definition with that default_related_name. I think additional classes with default_related_name = '_distributions' would cause an ambiguation error. Note that it needs to match the _distributions of PublicationSerializer here.

Yes, I wondered how this works as well. Your assumption why this works makes sense. Do we need the default_related_name in RepositoryVersionDistribution at all? And if so, do we need to set it to _distributions as well?

I think we do need it because we need to represent the backref in other objects like the PublicationSerializer. I don't think these backrefs will make themselves and if we don't specify them they'll just end up with stranger names.

Since we want them all to have unique names we need to remove these and have the actual detail methods name them. If we allow these to be inherited we'll end up with duplicates for sure. I'm removing these and adding it to FileDistribution now.

They'll also have to make their serializer, e.g. FilePublicationSerializer include the backref name to match default_related_name but that's just the way it is.

That sounds right to me.

I'm about to push a version that also updates the handler some. Let's see what you think of it.

django wouldn't start complaining about the ambiguity of _distributions between these two models specifically.

Why do you expect Django to complain in this case? My understanding is that Django 'injects' a kind of a pseudo field into the model/serializer of the model the foreign key is from. Thus, Publication, Repository, and RepositoryVersion each have a '_distributions' field that one can use in the respective serializer.

I just cobbled together something: pulp_cookbook has two distributor types (one for 'cookbook', one for 'cookbook2', RepositorySerializer has a _distributions field showing related RepositoryVersionDistribution models (obviously, this does not make any sense). And:

$ http :24817$PUBLICATION_HREF { "_created": "2019-05-09T22:30:06.686178Z", "_distributions": [ "/pulp/api/v3/distributions/cookbook/cookbook/95f06efb-5331-441d-bd08-459bc806ff30/" ], "_href": "/pulp/api/v3/publications/cookbook/cookbook/e003db72-78b9-4ce1-8fb2-066d92608170/", "_type": "cookbook.cookbook", "publisher": "/pulp/api/v3/publishers/cookbook/cookbook/e0ceb4a7-2dba-4267-8454-44b7dee8f073/", "repository": "http://localhost:24817/pulp/api/v3/repositories/%3CRepository:%20foo%3E/", "repository_version": "/pulp/api/v3/repositories/8f5b97e4-5485-4b4c-9ae4-f91a14c1a0a2/versions/1/" } $ http :24817$REPO_HREF { "_created": "2019-05-09T22:27:28.355982Z", "_distributions": [ "/pulp/api/v3/distributions/cookbook/cookbook2/f38d83a7-282d-457f-ab77-9f1c79d2df54/" ], "_href": "/pulp/api/v3/repositories/8f5b97e4-5485-4b4c-9ae4-f91a14c1a0a2/", "_latest_version_href": "/pulp/api/v3/repositories/8f5b97e4-5485-4b4c-9ae4-f91a14c1a0a2/versions/1/", "_versions_href": "/pulp/api/v3/repositories/8f5b97e4-5485-4b4c-9ae4-f91a14c1a0a2/versions/", "description": "", "name": "foo" }

Commit is here: gmbnomis@d80e3f3

I'm going to wait to merge this until I can do some testing also tomorrow morning.

@gmbnomis is the idea just to bring back PublicationSerializer._distributions to replace FilePublicationSerializer._file_distributions ?

I don't think we want to make models.PublicationDistribution and RepositoryVersionDistribution models non-abstract because then the inheritance tree becomes 3 large and with concrete tables that is too much for the ORM I think.

Yes, that is is the idea. You are right, we should avoid deep inheritance levels if possible. I think we should keep it as you implemented it and make the REST API nicer by a small adaptation on the plugin side: https://github.com/pulp/pulp_file/pull/219/files#r282762667

Model Changes The Distribution model was replaced with the RepositoryVersionDistribution and PublicationDistribution models. These are both abstract models and are meant to be subclassed further. Serializer Changes The DistributionSerializer is removed and now BaseDistributionSerializer, PublicationDistributionSerializer, and RepositoryVersionDistributionSerializer are here. These can also be further subclassed by plugin writers. ViewSet Changes DistributionViewset is now called BaseDistributionViewSet. No other viewsets are provided. Docs Changes The release notes are updated to indicate the breaking changes. Required PR: pulp/pulpcore#124 Required PR: pulp/pulp_file#219 https://pulp.plan.io/issues/4785 closes #4785

codecov · 2019-05-09T22:19:44Z

Codecov Report

Merging #124 into master will decrease coverage by 0.14%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
- Coverage   67.26%   67.11%   -0.15%     
==========================================
  Files          64       64              
  Lines        3030     3029       -1     
==========================================
- Hits         2038     2033       -5     
- Misses        992      996       +4

Impacted Files	Coverage Δ
pulpcore/app/serializers/__init__.py	`100% <ø> (ø)`	⬆️
pulpcore/app/models/__init__.py	`100% <ø> (ø)`	⬆️
pulpcore/app/viewsets/__init__.py	`100% <ø> (ø)`	⬆️
pulpcore/app/viewsets/publication.py	`100% <100%> (ø)`	⬆️
pulpcore/app/models/publication.py	`79.41% <100%> (+0.95%)`	⬆️
pulpcore/content/handler.py	`61.49% <61.53%> (ø)`	⬆️
pulpcore/app/serializers/publication.py	`91.66% <89.65%> (-3.58%)`	⬇️
pulpcore/app/serializers/task.py	`98.07% <0%> (-1.93%)`	⬇️

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 f0bd807...ee41552. Read the comment docs.

Model Changes The Distribution model was replaced with the RepositoryVersionDistribution and PublicationDistribution models. These are both abstract models and are meant to be subclassed further. Serializer Changes The DistributionSerializer is removed and now BaseDistributionSerializer, PublicationDistributionSerializer, and RepositoryVersionDistributionSerializer are here. These can also be further subclassed by plugin writers. ViewSet Changes DistributionViewset is now called BaseDistributionViewSet. No other viewsets are provided. Docs Changes The release notes are updated to indicate the breaking changes. Required PR: pulp/pulpcore#124 Required PR: pulp/pulp_file#219 https://pulp.plan.io/issues/4785 closes #4785

gmbnomis · 2019-05-10T06:51:40Z

LGTM. This improves the plugin API and REST API for Distribution a lot! And having the plugin writer implement a distributions field in the publication serializer is a minor nuisance compared to excessive joining in the content app's hot path.

nixocio · 2019-05-10T11:58:44Z

pulpcore/tests/functional/api/using_plugin/test_crd_publications.py

@@ -80,7 +80,7 @@ def test_02_read_publication_with_specific_fields(self):

        Permutate field list to ensure different combinations on result.
        """
-        fields = ('_href', '_created', '_distributions', 'publisher')
+        fields = ('_href', '_created', '_file_distributions', 'publisher')


Every plugin will have now a different field for this? like..._rpm_distributions and so on.

I'm going to switch this to distributions see here for more commentary: pulp/pulp_file#219 (comment)

The Distribution contained options which should not be mixed. Specifically the repository and repository_version options go together and the publication goes by itself. This PR splits that object into two new objects RepositoryVersionDistribution and PublicationDistribution. The two models are not detail objects, and require the plugin writer to declare a Distribution detail object. It was already this way before this PR. This also adds a release note about the breaking changes that come with the switch to Master/Detail. Required PR: pulp/pulpcore-plugin#97 Required PR: pulp/pulp_file#219 https://pulp.plan.io/issues/4785 closes pulp#4785

The plugin API now uses PublicationDistribution which causes pulp_file users to only receive the fields they can actually use. Required PR: pulp/pulpcore#124 Required PR: pulp/pulpcore-plugin#97 https://pulp.plan.io/issues/4785 closes #4785

bmbouter added Breaking Changes Work In Progress labels May 9, 2019

bmbouter force-pushed the split-distribution branch from 502f9d8 to c37854b Compare May 9, 2019 13:39

bmbouter mentioned this pull request May 9, 2019

Replace Distribution with other objects pulp/pulpcore-plugin#97

Merged

asmacdo reviewed May 9, 2019

View reviewed changes

pulpcore/app/viewsets/publication.py Outdated Show resolved Hide resolved

bmbouter mentioned this pull request May 9, 2019

Use PublicationDistribution from pulpcore.plugin pulp/pulp_file#219

Merged

bmbouter removed the Work In Progress label May 9, 2019

dkliban reviewed May 9, 2019

View reviewed changes

pulpcore/app/viewsets/publication.py Outdated Show resolved Hide resolved

bmbouter force-pushed the split-distribution branch 3 times, most recently from 5506711 to 2f1878c Compare May 9, 2019 17:11

bmbouter commented May 9, 2019

View reviewed changes

gmbnomis reviewed May 9, 2019

View reviewed changes

bmbouter commented May 9, 2019

View reviewed changes

bmbouter force-pushed the split-distribution branch 3 times, most recently from 76b3035 to d8122b8 Compare May 9, 2019 21:10

bmbouter requested a review from a team May 9, 2019 21:10

bmbouter force-pushed the split-distribution branch 2 times, most recently from 5190c62 to 77b0872 Compare May 9, 2019 22:07

dkliban approved these changes May 9, 2019

View reviewed changes

nixocio reviewed May 10, 2019

View reviewed changes

bmbouter force-pushed the split-distribution branch from 77b0872 to ee41552 Compare May 10, 2019 16:18

bmbouter merged commit 6cf33cf into pulp:master May 10, 2019

bmbouter deleted the split-distribution branch May 10, 2019 16:50

daviddavis mentioned this pull request May 14, 2019

Remove Publications pulp/pulp_ansible#96

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split Distribution into two objects #124

Split Distribution into two objects #124

bmbouter commented May 9, 2019 •

edited

Loading

bmbouter May 9, 2019

dkliban May 9, 2019

bmbouter May 9, 2019

bmbouter May 9, 2019

gmbnomis May 9, 2019

bmbouter May 9, 2019

gmbnomis May 9, 2019

bmbouter May 9, 2019

gmbnomis May 9, 2019

bmbouter May 9, 2019

bmbouter May 9, 2019

bmbouter May 9, 2019

gmbnomis May 9, 2019

bmbouter May 9, 2019

bmbouter May 9, 2019

dkliban May 9, 2019

bmbouter May 9, 2019

gmbnomis May 9, 2019

gmbnomis May 9, 2019 •

edited

Loading

bmbouter May 9, 2019

gmbnomis May 10, 2019

codecov bot commented May 9, 2019 •

edited

Loading

gmbnomis commented May 10, 2019

nixocio May 10, 2019

bmbouter May 10, 2019

		@@ -95,12 +99,12 @@ def async_reserved_resources(self, instance):
		@classmethod
		def is_master_viewset(cls):

Split Distribution into two objects #124

Split Distribution into two objects #124

Conversation

bmbouter commented May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmbnomis May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 9, 2019 • edited Loading

Codecov Report

gmbnomis commented May 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter commented May 9, 2019 •

edited

Loading

gmbnomis May 9, 2019 •

edited

Loading

codecov bot commented May 9, 2019 •

edited

Loading