Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

Create Serializers and Viewsets for Content #291

Merged
merged 2 commits into from Jan 8, 2019

Conversation

asmacdo
Copy link
Contributor

@asmacdo asmacdo commented Dec 19, 2018

This commit leaves POST (create) unimplemented as well as Filters which
are each complex enough that they warrent a separate issue and PR.

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

@asmacdo
Copy link
Contributor Author

asmacdo commented Dec 19, 2018

Added WIP so I remember to add all the help_texts

@asmacdo asmacdo force-pushed the add-content-viewsets-serializers branch from 5c2e903 to 017c5c7 Compare December 20, 2018 14:33
@pep8speaks
Copy link

pep8speaks commented Dec 20, 2018

Hello @asmacdo! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 07, 2019 at 15:39 Hours UTC

@asmacdo asmacdo force-pushed the add-content-viewsets-serializers branch 3 times, most recently from f89748d to 7f12da3 Compare December 20, 2018 15:27
Copy link
Member

@ipanova ipanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asmacdo what about size property on the serializers?
i think the plan was to retrieve this information from the Artifact/RemoteArtifact

pulp_docker/app/serializers.py Outdated Show resolved Hide resolved
@asmacdo asmacdo force-pushed the add-content-viewsets-serializers branch from 7f12da3 to b2d5736 Compare December 21, 2018 14:40
@@ -123,6 +123,9 @@ class BlobManifestBlob(models.Model):
manifest_blob = models.ForeignKey(
ManifestBlob, related_name='manifest_blobs', on_delete=models.CASCADE)

class Meta:
unique_together = ('manifest', 'manifest_blob')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After adding the viewsets, I found that the many-to-many relationships were being written multiple times. This made the serialization really ugly, and I wasn't sure it was working correctly, so I went ahead and fixed this problem since it was simple.

If preferred, I can break these changes into a different PR, but making the change makes it easier to check that the viewsets/serializers were working as expected

"""
Create a new ManifestListTag from a request.
"""
raise NotImplementedError()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the parent ViewSet includes POST (create), it is necessary to override this-- and because the upload functionality is sufficiently complex, I think it should be done in a separate issue/PR. I'll file that issue before I merge.

@asmacdo asmacdo force-pushed the add-content-viewsets-serializers branch from b2d5736 to cdc3237 Compare December 21, 2018 15:20
Even though Docker content can only have 1 Artifact, this field should be used with
`many=True` because the Artifact is related with the ContentArtifact through table, and when
the same Artifact is served at multiple relative_paths, there are multiple associations of
between a given Content and Artifact.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does an artifact get served at multiple relative paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only one that I investigated was tag names

This commit leaves POST (create) unimplemented as well as Filters which
are each complex enough that they warrent a separate issue and PR.

https://pulp.plan.io/issues/3989
fixes #3989
@asmacdo asmacdo force-pushed the add-content-viewsets-serializers branch 2 times, most recently from 1610d92 to 2bf5485 Compare January 7, 2019 14:37
@asmacdo
Copy link
Contributor Author

asmacdo commented Jan 7, 2019

Here's how a blob serializers, FYI

    {
        "_artifact": {
            "_href": "/pulp/api/v3/artifacts/331/",
            "size": 715296
        },
        "_created": "2019-01-07T13:11:25.349266Z",
        "_href": "/pulp/api/v3/content/docker/blobs/300/",
        "_type": "pulp_docker.manifest-blob",
        "digest": "sha256:0ffadd58f2a61468f527cc4f0fc45272ee4a1a428abe014546c89de2aa6a0eb5",
        "media_type": "application/vnd.docker.image.rootfs.diff.tar.gzip"
    }

Previously, ImageManifests that were tagged were had multiple relations
to the same artifact, 1 for the sha and 1 for each tag. This was caused
by sharing the DeclarativeArtifact between the tags and the
ImageManifest during sync.

After those relations were pruned, update the serializers and models to
a single-artifact Content.

[noissue]
@asmacdo asmacdo force-pushed the add-content-viewsets-serializers branch from 2bf5485 to 5544e14 Compare January 7, 2019 15:39
Copy link
Member

@dkliban dkliban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this out and it worked for me. Thanks!

@asmacdo
Copy link
Contributor Author

asmacdo commented Jan 8, 2019

There was some concern about some Manifests that associate the same blob more than once. I am thinking this problem was environmental however. I tested this with busybox and got the expected results.

If anyone encounters this problem, please have a look at the db using shell_plus.

 multiple_blobs = ImageManifest.objects.annotate(a_count=Count('blobs')).filter(a_count__gt=1)

This query returned 1 result, which was a Manifest that indeed had 2 blobs, but they were distinct.


~/d/devel ❯ http --auth admin:admin http://pulp3:8000/pulp/api/v3/content/docker/manifests/5/
HTTP/1.1 200 OK
Allow: GET, HEAD, OPTIONS
Content-Length: 503
Content-Type: application/json
Date: Tue, 08 Jan 2019 14:44:14 GMT
Server: WSGIServer/0.2 CPython/3.7.2
Vary: Accept, Cookie
X-Frame-Options: SAMEORIGIN

{
    "_artifact": {
        "_href": "/pulp/api/v3/artifacts/10/",
        "size": 711
    },
    "_created": "2019-01-08T14:31:11.893252Z",
    "_href": "/pulp/api/v3/content/docker/manifests/5/",
    "_type": "pulp_docker.manifest",
    "blobs": [
        "/pulp/api/v3/content/docker/blobs/106/",
        "/pulp/api/v3/content/docker/blobs/111/"
    ],
    "config_blob": "/pulp/api/v3/content/docker/blobs/109/",
    "digest": "sha256:19d091b7368bf6538f40439f9530b878ce3831f8ff46c8bd95d296509d1303e5",
    "media_type": "application/vnd.docker.distribution.manifest.v2+json",
    "schema_version": 2
}

~/d/devel ❯ http --auth admin:admin http://pulp3:8000/pulp/api/v3/content/docker/blobs/106/
HTTP/1.1 200 OK
Allow: GET, HEAD, OPTIONS
Content-Length: 337
Content-Type: application/json
Date: Tue, 08 Jan 2019 14:44:23 GMT
Server: WSGIServer/0.2 CPython/3.7.2
Vary: Accept, Cookie
X-Frame-Options: SAMEORIGIN

{
    "_artifact": {
        "_href": "/pulp/api/v3/artifacts/140/",
        "size": 32
    },
    "_created": "2019-01-08T14:31:15.884055Z",
    "_href": "/pulp/api/v3/content/docker/blobs/106/",
    "_type": "pulp_docker.manifest-blob",
    "digest": "sha256:a3ed95caeb02ffe68cdd9fd84406680ae93d633cb16422d00e8a7c22955b46d4",
    "media_type": "application/vnd.docker.image.rootfs.diff.tar.gzip"
}

~/d/devel ❯ http --auth admin:admin http://pulp3:8000/pulp/api/v3/content/docker/blobs/111/
HTTP/1.1 200 OK
Allow: GET, HEAD, OPTIONS
Content-Length: 341
Content-Type: application/json
Date: Tue, 08 Jan 2019 14:44:27 GMT
Server: WSGIServer/0.2 CPython/3.7.2
Vary: Accept, Cookie
X-Frame-Options: SAMEORIGIN

{
    "_artifact": {
        "_href": "/pulp/api/v3/artifacts/144/",
        "size": 686275
    },
    "_created": "2019-01-08T14:31:16.009351Z",
    "_href": "/pulp/api/v3/content/docker/blobs/111/",
    "_type": "pulp_docker.manifest-blob",
    "digest": "sha256:f5c4e6c495776bf839ba15ab1a68dc7ef2f491ec4c258431a107445812ecd65d",
    "media_type": "application/vnd.docker.image.rootfs.diff.tar.gzip"
}

@asmacdo asmacdo merged commit 4f97829 into pulp:master Jan 8, 2019
@asmacdo
Copy link
Contributor Author

asmacdo commented Jan 8, 2019

FWIW, I also tested this by running multiple syncs, and got the same results as above.

@ipanova
Copy link
Member

ipanova commented Jan 8, 2019

@asmacdo i confirm that it was env issue

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants