Add namespace to distributions #158

mdellweg · 2020-09-24T11:56:15Z

fixes #7089
https://pulp.plan.io/issues/7089

This is the first version to pass ci. Still missing are at least:

a data migration
~~- [ ] validation of namespaces to be collision free with non namespaced distribution~~ - The new design handles this on the database table level.

pulpbot · 2020-09-24T11:58:18Z

Attached issue: https://pulp.plan.io/issues/7089

mdellweg · 2020-09-24T13:01:13Z

I just realized, that if paths without a "/" still had a namespace (in that case with the same name), the uniqueness would be trivially given by the database.

ipanova · 2020-09-29T12:19:04Z

pulp_container/app/serializers.py

+        base_path = validated_data["base_path"]
+
+        if "namespace" in validated_data:
+            if not base_path.startswith(validated_data["namespace"].name + "/"):


ipanova · 2020-09-29T12:22:06Z

i know this change is not really related to the PR but can you please update and remove the publication reference https://github.com/pulp/pulp_container/pull/158/files#diff-90a314d7b2d5ddb20d360be02a4ec146R344

ipanova · 2020-10-06T16:52:44Z

pulp_container/app/viewsets.py

+    ViewSet for ContainerNamespaces.
+    """
+
+    endpoint_name = "pulp_container/namespaces"


the changes look good to me, @dkliban do you want to take a look? do you have a different suggestion on the endpoint or we leave this one as is?

i looked at 'django-admin show_urls' and i would not be opposed also having /namespaces/container/ as another option to consider

jlsherrill · 2020-10-12T15:54:02Z

pulp_container/app/serializers.py

+                namespace_serializer.is_valid(raise_exception=True)
+                validated_data["namespace"] = namespace_serializer.create(
+                    namespace_serializer.validated_data
+                )


there's room here for a race condition, if two repos are being created with a new namespace at the same time it could result in one of the 'creates' to fail when the other task created it. Not sure how often ya'll deal with that within pulp, but we see it all the time within katello

Sounds like we really need a sophisticated get_or_create on the serializer model.
I think we do that in >=3 places.

I added another except block to recognize a lost race.

ipanova · 2020-10-26T13:49:30Z

HTTP/1.1 200 OK
Allow: GET, POST, HEAD, OPTIONS
Connection: close
Content-Length: 112
Content-Type: application/json
Date: Mon, 26 Oct 2020 13:48:14 GMT
Server: gunicorn/20.0.4
Vary: Accept, Cookie
X-Frame-Options: SAMEORIGIN

{
    "count": 4,
    "next": null,
    "previous": null,
    "results": [

        {
            "name": "bar"
        },
        {
            "name": "foo"
        }
    ]
}

I suggest adding pulp_href pulp_created and maybe list of distributions

ipanova · 2020-10-26T12:53:54Z

pulp_container/app/serializers.py

+            result = cls.Meta.model.objects.get(**natural_key)
+        except ObjectDoesNotExist:
+            data = {}
+            if default_values is not None:


if default_values:

ipanova · 2020-10-26T13:10:46Z

pulp_container/app/models.py

    """

    TYPE = "container"

+    namespace = models.ForeignKey(
+        ContainerNamespace,
+        on_delete=models.CASCADE,


ipanova · 2020-10-26T13:45:25Z

pulp_container/app/serializers.py

+            data = {}
+            if default_values is not None:
+                data.update(default_values)
+            data.update(natural_key)


data seems to be unused

But it should be... Changing that.

ipanova · 2020-10-26T15:06:42Z

pulp_container/app/serializers.py

+    """
+
+    class Meta:
+        fields = ModelSerializer.Meta.fields + ["name"]


we should use a tuple here instead of list

mdellweg · 2020-10-26T15:10:17Z

I suggest adding pulp_href pulp_created and maybe list of distributions

As the list of distributions can get very large, i think it would be better to make the distribution filterable by namespace.

ipanova · 2020-10-26T15:23:39Z

I suggest adding pulp_href pulp_created and maybe list of distributions

As the list of distributions can get very large, i think it would be better to make the distribution filterable by namespace.

That's a fine idea, for that we would need to add NamespaceFilter to the DistributionViewset.
Right now we are not serializing the namespace on the distribution, should we, given that ^ change?

ipanova · 2020-11-02T17:33:44Z

while i can filter the distributions by the name of the namespace i get 500 on the namespace endpoint

$ http GET :24817/pulp/api/v3/distributions/container/container/?namespace__name=my-repo1
HTTP/1.1 200 OK
Allow: GET, POST, HEAD, OPTIONS
Connection: close
Content-Length: 637
Content-Type: application/json
Date: Mon, 02 Nov 2020 17:32:35 GMT
Server: gunicorn/20.0.4
Vary: Accept, Cookie
X-Frame-Options: SAMEORIGIN

{
    "count": 1,
    "next": null,
    "previous": null,
    "results": [
        {
            "base_path": "my-repo1",
            "content_guard": "/pulp/api/v3/contentguards/container/content_redirect/acbae288-e7cf-471f-bbd7-e88bb27300a6/",
            "name": "my-repo1",
            "namespace": "/pulp/api/v3/pulp_container/namespaces/4e565f62-aec7-48bb-8746-80cd412a69b5/",
            "pulp_created": "2020-11-02T17:27:59.566894Z",
            "pulp_href": "/pulp/api/v3/distributions/container/container/7252915c-53f6-4ba4-bcec-caff34c25d64/",
            "registry_path": "pulp3-source-fedora32.fluffy.example.com/my-repo1",
            "repository": "/pulp/api/v3/repositories/container/container-push/ade7c212-edee-4253-af06-738c6449862c/",
            "repository_version": null
        }
    ]
}

(pulp) [vagrant@pulp3-source-fedora32 _scripts]$ http GET :24817/pulp/api/v3/pulp_container/namespaces/4e565f62-aec7-48bb-8746-80cd412a69b5/
HTTP/1.1 500 Internal Server Error
Connection: close
Content-Length: 27
Content-Type: text/html
Date: Mon, 02 Nov 2020 17:32:41 GMT
Server: gunicorn/20.0.4
Vary: Cookie
X-Frame-Options: SAMEORIGIN

<h1>Server Error (500)</h1>

mdellweg · 2020-11-04T09:57:12Z

Somehow the IdentityField derives the view as "containernamespace-detail", while it is "pulp_container/namespaces-detail". This needs more investigation.

mdellweg · 2020-11-04T10:10:21Z

pulp_container/app/serializers.py

+    Serializer for ContainerNamespaces.
+    """
+
+    pulp_href = IdentityField(view_name="pulp_container/namespaces-detail")


@ipanova this should fix it.

fixes #7089 https://pulp.plan.io/issues/7089

[noissue]

mdellweg force-pushed the namespaces branch from 50e4d0f to 91d6204 Compare September 24, 2020 12:39

mdellweg force-pushed the namespaces branch 2 times, most recently from 8091149 to d2db1fc Compare September 28, 2020 11:10

ipanova reviewed Sep 29, 2020

View reviewed changes

mdellweg force-pushed the namespaces branch 3 times, most recently from 2cc876b to 8d56ba8 Compare September 30, 2020 14:37

mdellweg changed the title ~~WIP: Add namespace to distributions~~ Add namespace to distributions Oct 1, 2020

mdellweg force-pushed the namespaces branch from 8d56ba8 to f1f76dd Compare October 1, 2020 15:34

ipanova reviewed Oct 6, 2020

View reviewed changes

jlsherrill reviewed Oct 12, 2020

View reviewed changes

mdellweg force-pushed the namespaces branch 3 times, most recently from 589931d to b1bdaf9 Compare October 20, 2020 12:25

mdellweg marked this pull request as ready for review October 20, 2020 12:27

mdellweg force-pushed the namespaces branch from b1bdaf9 to bd3bc0e Compare October 20, 2020 12:50

ipanova reviewed Oct 26, 2020

View reviewed changes

mdellweg force-pushed the namespaces branch from bd3bc0e to 20e8fc5 Compare October 26, 2020 14:08

ipanova reviewed Oct 26, 2020

View reviewed changes

mdellweg force-pushed the namespaces branch from 20e8fc5 to 8ed3c6d Compare October 26, 2020 15:07

mdellweg force-pushed the namespaces branch 2 times, most recently from 47e9faf to ba009c8 Compare November 4, 2020 10:09

mdellweg commented Nov 4, 2020

View reviewed changes

mdellweg added 2 commits November 4, 2020 11:16

Add namespace to distributions

77e7166

fixes #7089 https://pulp.plan.io/issues/7089

Allow to filter distributions by namespace

b9bbb0c

[noissue]

mdellweg force-pushed the namespaces branch from ba009c8 to b9bbb0c Compare November 4, 2020 10:17

ipanova approved these changes Nov 4, 2020

View reviewed changes

mdellweg merged commit 749d509 into pulp:master Nov 4, 2020

mdellweg deleted the namespaces branch November 4, 2020 12:36

Add namespace to distributions #158

Add namespace to distributions #158

mdellweg commented Sep 24, 2020 •

edited

pulpbot commented Sep 24, 2020

mdellweg commented Sep 24, 2020

ipanova Sep 29, 2020

ipanova commented Sep 29, 2020 •

edited

ipanova Oct 6, 2020

ipanova Oct 26, 2020

jlsherrill Oct 12, 2020

mdellweg Oct 12, 2020

mdellweg Oct 21, 2020

ipanova commented Oct 26, 2020

ipanova Oct 26, 2020

ipanova Oct 26, 2020

ipanova Oct 26, 2020

mdellweg Oct 26, 2020

ipanova Oct 26, 2020

mdellweg commented Oct 26, 2020

ipanova commented Oct 26, 2020 •

edited

ipanova commented Nov 2, 2020

mdellweg commented Nov 4, 2020 •

edited

mdellweg Nov 4, 2020

Add namespace to distributions #158

Add namespace to distributions #158

Conversation

mdellweg commented Sep 24, 2020 • edited

pulpbot commented Sep 24, 2020

mdellweg commented Sep 24, 2020

Choose a reason for hiding this comment

ipanova commented Sep 29, 2020 • 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

ipanova commented Oct 26, 2020

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

mdellweg commented Oct 26, 2020

ipanova commented Oct 26, 2020 • edited

ipanova commented Nov 2, 2020

mdellweg commented Nov 4, 2020 • edited

Choose a reason for hiding this comment

mdellweg commented Sep 24, 2020 •

edited

ipanova commented Sep 29, 2020 •

edited

ipanova commented Oct 26, 2020 •

edited

mdellweg commented Nov 4, 2020 •

edited