API Changes #3774

dralley · 2018-11-27T20:28:42Z

These are the API changes from the single-table-content work that were discussed.

pep8speaks · 2018-11-27T20:28:47Z

Hello @dralley! Thanks for updating the PR.

There are no PEP8 issues in the file pulpcore/app/models/repository.py !
There are no PEP8 issues in the file pulpcore/app/serializers/__init__.py !
There are no PEP8 issues in the file pulpcore/app/serializers/base.py !
There are no PEP8 issues in the file pulpcore/app/serializers/repository.py !
There are no PEP8 issues in the file pulpcore/app/serializers/task.py !
There are no PEP8 issues in the file pulpcore/app/util.py !
There are no PEP8 issues in the file pulpcore/app/viewsets/__init__.py !
There are no PEP8 issues in the file pulpcore/app/viewsets/content.py !
There are no PEP8 issues in the file pulpcore/app/viewsets/custom_filters.py !
There are no PEP8 issues in the file pulpcore/app/viewsets/repository.py !
There are no PEP8 issues in the file pulpcore/tasking/util.py !
There are no PEP8 issues in the file pulpcore/tests/functional/api/using_plugin/constants.py !
There are no PEP8 issues in the file pulpcore/tests/functional/api/using_plugin/test_auto_distribution.py !
There are no PEP8 issues in the file pulpcore/tests/functional/api/using_plugin/test_content_promotion.py !
There are no PEP8 issues in the file pulpcore/tests/functional/api/using_plugin/test_orphans.py !
There are no PEP8 issues in the file pulpcore/tests/functional/api/using_plugin/test_repo_versions.py !
There are no PEP8 issues in the file pulpcore/tests/functional/api/using_plugin/test_tasking.py !
There are no PEP8 issues in the file pulpcore/tests/functional/api/using_plugin/test_unlinking_repo.py !

Comment last updated on December 14, 2018 at 19:58 Hours UTC

dralley · 2018-11-27T20:33:16Z

pulpcore/pulpcore/app/models/repository.py

@@ -247,6 +247,20 @@ def content(self):
        )
        return Content.objects.filter(version_memberships__in=relationships)

+    @property
+    def content_by_type(self):


I'm not actually sure this function (content_by_type) is necessary or even particularly useful, I may remove it before merging. It seemed like an obvious thing to have at first but I realized that I can't think of anywhere it would actually be used. Generic code doesn't usually need to touch the content-specific fields, so it can just use Content objects. Plugin code already knows what concrete models exist so it can just manage those directly also.

Should the added() and removed() methods be made properties, to match content? Or should we make content a method? The latter would provide maximal flexibility in terms of being able to add functionality later on in the form of kwargs.

dralley · 2018-11-28T21:36:46Z

pulpcore/pulpcore/app/models/repository.py

@@ -247,6 +249,62 @@ def content(self):
        )
        return Content.objects.filter(version_memberships__in=relationships)

+    @property


These 3 properties are probably temporary, pending a better solution. This is ugly but it was fairly simple for an initial implementation.

If anyone has suggestions on how to do this in a way that better respects separation of responsibilities, please let me know :) It probably isn't something we want to leave in the model layer.

dralley · 2018-11-29T18:28:15Z

pulpcore/pulpcore/app/serializers/repository.py

-    _content_href = NestedIdentityField(
-        view_name='versions-content',
-        lookup_field='number', parent_lookup_kwargs={'repository_pk': 'repository__pk'},
+    _content = serializers.DictField(


Are these names OK or should we go back to what they were before?

Different variants:

"added_content"

"content_added_hrefs"

"added_content_hrefs"

"added_hrefs"

I prefer the "added_content" and "removed_content".

dralley · 2018-12-03T04:21:18Z

pulpcore/pulpcore/app/serializers/base.py

@@ -244,35 +177,6 @@ class Meta:
    class Meta:
        fields = ModelSerializer.Meta.fields + ('type',)

-    def to_representation(self, instance):


This can be removed since we're no longer trying to serialize generic Content objects as their detail counterparts. Yay!

dralley · 2018-12-03T04:25:38Z

pulpcore/pulpcore/app/serializers/repository.py

-    _removed_href = NestedIdentityField(
-        view_name='versions-removed-content',
-        lookup_field='number', parent_lookup_kwargs={'repository_pk': 'repository__pk'},
+    _content_removed = serializers.DictField(


I assume the permanent solution to the aforementioned separation of concerns issue will be to make custom field types with the same logic as the properties, but I haven't done this yet and want to field suggestions first. This way was more straightforwards.

I agree that custom fields would be appropriate here.

dralley · 2018-12-03T04:29:54Z

pulpcore/pulpcore/app/viewsets/__init__.py

@@ -1,10 +1,16 @@
 from .base import AsyncRemoveMixin, AsyncUpdateMixin, BaseFilterSet, NamedModelViewSet  # noqa
+# These need to be imported prior to ContentViewSet - do not shuffle this by alphabetical order
+from .custom_filters import (  # noqa


I struggled a little bit with circular imports here -- should we just move these to viewsets.content since they're only used in that one place?

I don't see a problem with moving them there.

dralley · 2018-12-03T04:31:02Z

pulpcore/pulpcore/app/viewsets/custom_filters.py

@@ -60,3 +63,101 @@ class IsoDateTimeFilter(DateTimeFilter):
    def __init__(self, *args, **kwargs):
        kwargs.setdefault('help_text', _('ISO 8601 formatted dates are supported'))
        super().__init__(*args, **kwargs)
+
+
+class RepoVersionHrefFilter(Filter):


Should I rename this to "BaseRepoVersionHrefFilter" since it's meant to be subclassed and not used directly?

I don't much like the naming of these, generally. Naming suggestions?

The current name seems fine. The not implemented on filter() will let a developer know not to use it directly.

dralley · 2018-12-03T04:36:20Z

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

@@ -110,13 +110,13 @@ def test_02_sync_content(self):

        self.assertIsNotNone(repo['_latest_version_href'])

-        content = get_content(repo)
+        content = get_file_content(repo)


As mentioned on the pulp_file PR, we could just always do get_content(repo)['file'] instead. Open to changing this if QE would prefer it that way.

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added Work In Progress 3.0 Issue labels Nov 27, 2018

dralley commented Nov 27, 2018

View reviewed changes

dralley force-pushed the api-changes branch 13 times, most recently from f8b232a to 51e216d Compare November 28, 2018 21:33

dralley commented Nov 28, 2018

View reviewed changes

dralley force-pushed the api-changes branch from 51e216d to 90dfbe1 Compare November 28, 2018 21:59

dralley removed the Work In Progress label Nov 28, 2018

dralley changed the title ~~API Changes [WIP]~~ API Changes Nov 28, 2018

dralley commented Nov 29, 2018

View reviewed changes

dralley commented Dec 3, 2018

View reviewed changes

dralley force-pushed the api-changes branch from 90dfbe1 to 3e235a8 Compare December 4, 2018 16:49

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 17, 2018

Add back support for Required PR with Pulp Smash

7191478

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 17, 2018

Add back support for Required PR with Pulp Smash

fa2afdd

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 17, 2018

Add back support for Required PR with Pulp Smash

7168d53

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 17, 2018

Add back support for Required PR with Pulp Smash

ff29f65

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 17, 2018

Add back support for Required PR with Pulp Smash

d5f43fb

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_rpm that referenced this pull request Dec 17, 2018

Adapting Pulp Smash tests to new schema

ae9aa7e

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley mentioned this pull request Dec 17, 2018

API changes pulp/pulp_rpm#1248

Merged

dralley added a commit to dralley/pulp_rpm that referenced this pull request Dec 17, 2018

Adapting Pulp Smash tests to new schema

6a10516

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley mentioned this pull request Dec 17, 2018

API Changes pulp/pulp-smash#1154

Merged

dralley added a commit to dralley/pulp_rpm that referenced this pull request Dec 17, 2018

Adapting Pulp Smash tests to new schema

0d256a8

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

nixocio approved these changes Dec 17, 2018

View reviewed changes

dralley added a commit to dralley/pulp_rpm that referenced this pull request Dec 17, 2018

Adapting Pulp Smash tests to new schema

97cbc23

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 17, 2018

Add back support for Required PR with Pulp Smash

2286025

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley merged commit 28a0f2e into pulp:master Dec 17, 2018

dralley deleted the api-changes branch December 17, 2018 21:52

dralley added a commit to dralley/pulp_python that referenced this pull request Dec 17, 2018

Adapting pulp_smash tests to new schema

ad88680

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley mentioned this pull request Dec 17, 2018

Adapting pulp_smash tests to new schema pulp/pulp_python#214

Merged

dralley added a commit to dralley/pulp_python that referenced this pull request Dec 17, 2018

Adapting pulp_smash tests to new schema

15f975b

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_python that referenced this pull request Dec 17, 2018

Adapting pulp_smash tests to new schema

1c303c2

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_python that referenced this pull request Dec 17, 2018

Adapting pulp_smash tests to new schema

2b30eea

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_ansible that referenced this pull request Dec 17, 2018

Adapting pulp_smash tests to new schema

1fb5bf0

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley mentioned this pull request Dec 17, 2018

Adapting pulp_smash tests to new schema pulp/pulp_ansible#73

Merged

dralley added a commit to dralley/pulp_python that referenced this pull request Dec 17, 2018

Adapting pulp_smash tests to new schema

577a1c0

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_ansible that referenced this pull request Dec 17, 2018

Adapting pulp_smash tests to new schema

a190a7c

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_ansible that referenced this pull request Dec 17, 2018

Adapting pulp_smash tests to new schema

e9304de

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_python that referenced this pull request Dec 17, 2018

Adapting pulp_smash tests to new schema

f9d665e

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_python that referenced this pull request Dec 17, 2018

Adapting pulp_smash tests to new schema

6de5d61

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

dralley added a commit to dralley/pulp_python that referenced this pull request Dec 18, 2018

Adapting pulp_smash tests to new schema

8762c85

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

daviddavis pushed a commit to daviddavis/pulp_file that referenced this pull request May 14, 2019

Adapting pulp_smash tests to new schema

7b66515

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

daviddavis pushed a commit to daviddavis/pulp_file that referenced this pull request May 14, 2019

Add back support for Required PR with Pulp Smash

1a67009

Required PR: pulp/pulp#3774 Required PR: pulp/pulp-smash#1154 [noissue]

API Changes #3774

API Changes #3774

dralley commented Nov 27, 2018

pep8speaks commented Nov 27, 2018 •

edited

dralley Nov 27, 2018 •

edited

dralley Nov 27, 2018

dralley Nov 28, 2018 •

edited

dralley Nov 29, 2018 •

edited

dkliban Dec 6, 2018

dralley Dec 3, 2018

dralley Dec 3, 2018

dkliban Dec 6, 2018

dralley Dec 3, 2018

dkliban Dec 6, 2018

dralley Dec 3, 2018 •

edited

dkliban Dec 6, 2018

dralley Dec 3, 2018 •

edited

API Changes #3774

API Changes #3774

Conversation

dralley commented Nov 27, 2018

pep8speaks commented Nov 27, 2018 • edited

Comment last updated on December 14, 2018 at 19:58 Hours UTC

dralley Nov 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley Nov 28, 2018 • edited

Choose a reason for hiding this comment

dralley Nov 29, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley Dec 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley Dec 3, 2018 • edited

Choose a reason for hiding this comment

pep8speaks commented Nov 27, 2018 •

edited

dralley Nov 27, 2018 •

edited

dralley Nov 28, 2018 •

edited

dralley Nov 29, 2018 •

edited

dralley Dec 3, 2018 •

edited

dralley Dec 3, 2018 •

edited