Adds new `Distribution` MasterModel #1198

bmbouter · 2021-03-19T20:56:15Z

Added the following new objects related to a new Distribution
MasterModel:

pulpcore.plugin.models.Distribution - A new MasterModel
Distribution which replaces the
pulpcore.plugin.models.BaseDistribution. This now contains the
repository, repository_version, and publication fields on
the MasterModel instead of on the detail models as was done with
pulpcore.plugin.models.BaseDistribution.
pulpcore.plugin.serializer.DistributionSerializer - A serializer
plugin writers should use with the new
pulpcore.plugin.models.Distribution.
pulpcore.plugin.viewset.DistributionViewSet - The viewset that
replaces the deprecated
pulpcore.plugin.viewset.BaseDistributionViewSet.
pulpcore.plugin.viewset.NewDistributionFilter - The filter that
pairs with the Distribution model.

closes #8384

pulpbot · 2021-03-19T20:57:04Z

WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue.

dralley · 2021-03-20T19:52:41Z

pulpcore/app/serializers/publication.py

+
+        if error:
+            msg = _(
+                "The attributes 'repository' and 'repository_version' must be used exclusively."


This doesn't necessarily need to be true. We could have the same semantics as publication-based distributions, where one particular {publication | repository_version} is being pointed to, but the existence of "repository" or not tells it whether or not to auto-distribute.

I'm not opposed to this but we should have a good justification for the different semantics.

Yeah this was a copy/paste from what we had. The thing really is that we can't have a distribution point to both a repository and a publication at the same time. Let me try to work on this validation to make that true instead and tell me what you think.

pulpcore/app/serializers/publication.py

bmbouter · 2021-03-24T19:49:02Z

I added the overlapping cross-model base path support. I'm hand-testing it because pulpcore's CI doesn't have two plugins with heterogeneous Distribution types. This code isn't going to be around long enough to justify adding two plugins for dedicated tests. So what I did was:

Install pulp_file with this PR checked out: Switches FileDistribution use Distribution pulp_file#481
Made a pulp_file distribution with pulp file distribution create --name mydistro --base-path foo
Created an ansible repo (needed to create an ansible distribution). pulp ansible repository create --name ansiblerepo

Created a distribution and correctly received the overlapping error cross-model.

pulp ansible distribution create --name asdf --base-path foo/bar --repository ansiblerepo
Error: {"base_path":["Overlaps with existing distribution 'mydistro'"]}

bmbouter · 2021-03-24T20:30:53Z

I added the overlapping cross-model name uniqueness support. I'm hand-testing it because pulpcore's CI doesn't have two plugins with heterogeneous Distribution types. This code isn't going to be around long enough to justify adding two plugins for dedicated tests. So what I did was:

Install pulp_file with this PR checked out: Switches FileDistribution use Distribution pulp_file#481
Made a pulp_file distribution with pulp file distribution create --name mydistro --base-path foo
Created an ansible repo (needed to create an ansible distribution). pulp ansible repository create --name ansiblerepo

Created a distribution and correctly received the overlapping error cross-model.

pulp ansible distribution create --name mydistro --base-path another --repository ansiblerepo
Error: {"name":["This field must be unique."]}

pulpcore/app/models/publication.py

dralley · 2021-03-24T23:48:50Z

pulpcore/app/models/publication.py

+
+    Subclasses are expected to use either the ``publication`` or ``repository_version`` field, but
+    not both. The content app that serves content is not prepared to serve content both ways at the
+    same time.


I wish we could unify this either by attaching publication to repository version, or using publication.pass_through for "RepositoryVersionDistributions"

No action item here, just a thought.

It would be nice to unify, but some plugins don't need a publication at all so I'm not sure creating an object would make sense to them. I see where you're coming from though. Either way maybe we can still unify in the future somehow.

pulpcore/app/serializers/publication.py

pulpcore==3.12 introduces a new MasterModel named `Distribution` designed to replace the `BaseDistribution` MasterModel. This PR switches the `FileDistribution` to use `Distribution`. It also ships a migration which moves the data from the `BaseDistribution` table to the `Distribution` field. Required PR: pulp/pulpcore#1198 closes #8387

bmbouter · 2021-03-30T15:05:18Z

pulpcore/app/serializers/publication.py

+
+
+class DistributionRepositoryVersionSerializerMixin:
+    repository = DetailRelatedField(


I think repository needs to either be copied to DistributionPublicationSerializerMixin or it needs to become its own mixin. @daviddavis @dralley @mdellweg wdyt?

Why should it be copied if it's now present and usable by both? I would think it would be part of the standard distribution serializer.

Won't the help text be slightly different for publication distributions vs repo version distributions?

I don't think it necessarily has to be. Something like The latest version of this repository will be distributed automatically is still true even if there's a publication step that happens in the middle.

It's actually not even true that the latest publication is what gets served, at least not atm.

I'm moving repository to DistributionSerializer itself. Also I have to remove these Mixins because the tests point out that we can't create a distribution that has a publication due to the error: {"publication":["Invalid hyperlink - Incorrect URL match."]}.

To work around that I tried setting:

view_name_pattern=r"publication(-.*/.*)?-detail",

view_name_pattern=r"publications(-.*/.*)?-detail",

view_name_pattern="publication-detail",

None of ^ worked.

What is the consequence of removing the mixins, unnecessary fields?

Plugins will have to copy the block now shown in the docstring of DistributionSerializer. We can't provide a de-duplicated copy.

pulpcore==3.12 introduces a new MasterModel named `Distribution` designed to replace the `BaseDistribution` MasterModel. This PR switches the `FileDistribution` to use `Distribution`. It also ships a migration which moves the data from the `BaseDistribution` table to the `Distribution` field. Required PR: pulp/pulpcore#1198 closes #8387

bmbouter · 2021-03-30T19:10:14Z

FYI I wrote up the two issues to resolve the NewDistributionFilter name this PR introduces:

Rename it when we remove the current DistributionFilter: https://pulp.plan.io/issues/8480
Eventually remove it: https://pulp.plan.io/issues/8479

pulpcore==3.12 introduces a new MasterModel named `Distribution` designed to replace the `BaseDistribution` MasterModel. This PR switches the `FileDistribution` to use `Distribution`. It also ships a migration which moves the data from the `BaseDistribution` table to the `Distribution` field. Required PR: pulp/pulpcore#1198 closes #8387

mdellweg · 2021-03-31T08:57:21Z

pulpcore/app/serializers/publication.py

+        validators=[
+            UniqueValidator(queryset=models.BaseDistribution.objects.all()),
+            UniqueValidator(queryset=models.Distribution.objects.all()),
+        ],


pulpcore/app/serializers/publication.py

pulpcore==3.12 introduces a new MasterModel named `Distribution` designed to replace the `BaseDistribution` MasterModel. This PR switches the `FileDistribution` to use `Distribution`. It also ships a migration which moves the data from the `BaseDistribution` table to the `Distribution` field. Required PR: pulp/pulpcore#1198 closes #8387

daviddavis

Looks good to me.

bmbouter · 2021-03-31T19:45:30Z

pulpcore/app/serializers/publication.py

+    def validate(self, data):
+        super().validate(data)
+
+        publication_in_data = "publication" in data


@dralley check this portion out, it's revised.

dralley · 2021-03-31T19:53:12Z

pulpcore/app/serializers/publication.py

+
+        if error:
+            msg = _(
+                "The attributes 'publication' and 'repository_version' must be used exclusively."


This is very slightly unclear I think. Maybe say something like

Only one of the attributes 'publication' and 'repository_version' may be used simultaneously.

Added the following new objects related to a new ``Distribution`` MasterModel: * ``pulpcore.plugin.models.Distribution`` - A new MasterModel ``Distribution`` which replaces the ``pulpcore.plugin.models.BaseDistribution``. This now contains the ``repository``, ``repository_version``, and ``publication`` fields on the MasterModel instead of on the detail models as was done with ``pulpcore.plugin.models.BaseDistribution``. * ``pulpcore.plugin.serializer.DistributionSerializer`` - A serializer plugin writers should use with the new ``pulpcore.plugin.models.Distribution``. * ``pulpcore.plugin.viewset.DistributionViewSet`` - The viewset that replaces the deprecated ``pulpcore.plugin.viewset.BaseDistributionViewSet``. * ``pulpcore.plugin.viewset.NewDistributionFilter`` - The filter that pairs with the ``Distribution`` model. closes #8384

dralley · 2021-04-01T19:34:15Z

pulpcore/app/serializers/publication.py

+        warnings.warn(
+            _(
+                "BaseDistributionSerializer is deprecated and could be removed as early as "
+                "pulpcore==3.13; use pulpcore.plugin.serializers.DistributionSerializer instead."


I would almost suggest putting a link to the Pulp File PR in here since it is a fairly "involved" migration process.

Actually one of the other ones calls out the migration specifically. I'm going to merge for now if that's alright.

dralley

See suggestion, otherwise LGTM

pulpcore==3.12 introduces a new MasterModel named `Distribution` designed to replace the `BaseDistribution` MasterModel. This PR switches the `FileDistribution` to use `Distribution`. It also ships a migration which moves the data from the `BaseDistribution` table to the `Distribution` field. Required PR: pulp/pulpcore#1198 closes #8387

dralley reviewed Mar 20, 2021

View reviewed changes

mdellweg reviewed Mar 22, 2021

View reviewed changes

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

bmbouter mentioned this pull request Mar 24, 2021

Switches FileDistribution use Distribution pulp/pulp_file#481

Closed

dralley reviewed Mar 24, 2021

View reviewed changes

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

dralley reviewed Mar 24, 2021

View reviewed changes

pulpcore/app/serializers/publication.py Show resolved Hide resolved

bmbouter force-pushed the new-distribution-master-model branch from 669e4fb to 9fda294 Compare March 25, 2021 13:08

This was referenced Mar 25, 2021

Switches FileDistribution use Distribution pulp/pulp_file#482

Closed

Switches FileDistribution use Distribution pulp/pulp_file#484

Merged

bmbouter force-pushed the new-distribution-master-model branch 2 times, most recently from 4a52067 to c9fcfdf Compare March 26, 2021 18:48

bmbouter commented Mar 30, 2021

View reviewed changes

bmbouter force-pushed the new-distribution-master-model branch from c9fcfdf to 48b1874 Compare March 30, 2021 15:10

bmbouter changed the title ~~New Distribution MasterModel~~ Adds new Distribution MasterModel Mar 30, 2021

bmbouter force-pushed the new-distribution-master-model branch 3 times, most recently from 7aa903e to 7351dda Compare March 30, 2021 15:54

bmbouter marked this pull request as ready for review March 30, 2021 15:55

bmbouter force-pushed the new-distribution-master-model branch 3 times, most recently from 092a2f0 to 6ed39c5 Compare March 30, 2021 19:01

bmbouter force-pushed the new-distribution-master-model branch 2 times, most recently from 7aae22f to b0db519 Compare March 30, 2021 19:50

mdellweg reviewed Mar 31, 2021

View reviewed changes

bmbouter force-pushed the new-distribution-master-model branch from b0db519 to 319cdb9 Compare March 31, 2021 12:56

daviddavis approved these changes Mar 31, 2021

View reviewed changes

bmbouter force-pushed the new-distribution-master-model branch 2 times, most recently from 461decc to d5ce7d1 Compare March 31, 2021 19:44

bmbouter commented Mar 31, 2021

View reviewed changes

dralley reviewed Mar 31, 2021

View reviewed changes

bmbouter force-pushed the new-distribution-master-model branch 2 times, most recently from dfa89ed to 3142a24 Compare April 1, 2021 15:19

bmbouter force-pushed the new-distribution-master-model branch from 3142a24 to d92b6ea Compare April 1, 2021 18:38

dralley reviewed Apr 1, 2021

View reviewed changes

dralley approved these changes Apr 1, 2021

View reviewed changes

bmbouter merged commit d55f1c2 into pulp:master Apr 1, 2021

bmbouter deleted the new-distribution-master-model branch April 1, 2021 19:41

Adds new `Distribution` MasterModel #1198

Adds new `Distribution` MasterModel #1198

bmbouter commented Mar 19, 2021 •

edited

pulpbot commented Mar 19, 2021

dralley Mar 20, 2021

bmbouter Mar 31, 2021

bmbouter commented Mar 24, 2021 •

edited

bmbouter commented Mar 24, 2021

dralley Mar 24, 2021 •

edited

bmbouter Mar 31, 2021

bmbouter Mar 30, 2021

dralley Mar 30, 2021

daviddavis Mar 30, 2021

dralley Mar 30, 2021 •

edited

bmbouter Mar 30, 2021

dralley Mar 30, 2021

bmbouter Mar 30, 2021

bmbouter commented Mar 30, 2021

mdellweg Mar 31, 2021

daviddavis left a comment

bmbouter Mar 31, 2021

dralley Mar 31, 2021 •

edited

dralley Apr 1, 2021

bmbouter Apr 1, 2021

dralley left a comment



		class DistributionRepositoryVersionSerializerMixin:
		repository = DetailRelatedField(

Adds new Distribution MasterModel #1198

Adds new Distribution MasterModel #1198

Conversation

bmbouter commented Mar 19, 2021 • edited

pulpbot commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter commented Mar 24, 2021 • edited

bmbouter commented Mar 24, 2021

dralley Mar 24, 2021 • edited

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

dralley Mar 30, 2021 • edited

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

bmbouter commented Mar 30, 2021

Choose a reason for hiding this comment

daviddavis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley Mar 31, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley left a comment

Choose a reason for hiding this comment

Adds new `Distribution` MasterModel #1198

Adds new `Distribution` MasterModel #1198

bmbouter commented Mar 19, 2021 •

edited

bmbouter commented Mar 24, 2021 •

edited

dralley Mar 24, 2021 •

edited

dralley Mar 30, 2021 •

edited

dralley Mar 31, 2021 •

edited