Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New endpoint returning all collections unpaginated #488

Merged
merged 2 commits into from Jan 28, 2021
Merged

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Jan 8, 2021

@pulpbot
Copy link
Member

pulpbot commented Jan 8, 2021

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

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

@@ -0,0 +1,2 @@
Introduce a new ``v3/metadata/<str:namespace>/<str:name>/versions/`` endpoint returning
Copy link
Member

Choose a reason for hiding this comment

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

Is this what it actually does? I though for 7941 it would list an unpaginated view of all CollectionVersions, but it would be v3/metadata/collection_versions/ not v3/metadata/<str:namespace>/<str:name>/versions/.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was unsure about this endpoint,
if it was all collections versions for a given collection, or all collection versions.
So I coded the first approach, thinking I had to mimic what we already have but unpaginated.
I'll work on it and change the endpoint!

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

This changelog entry needs updating.

Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

requested change to the implementation of metadata/collection_versions/.

@fao89 fao89 force-pushed the 7940 branch 4 times, most recently from 86e0fa4 to cffd018 Compare January 11, 2021 23:03
@fao89 fao89 requested a review from bmbouter January 12, 2021 13:08
Comment on lines -102 to -106
artifact = ContentArtifact.objects.get(content=instance)

context = self.get_serializer_context()
context["content_artifact"] = artifact
Copy link
Member Author

Choose a reason for hiding this comment

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

@bmbouter CollectionVersionViewSet only presented artifact data on retrieve, so the strategy was getting the ContentArtifact and injecting it into the context.
For UnpaginatedCollectionVersionViewSet we need to present artifact data on list, so I changed the query to select related ContentArtifact

Copy link
Member

Choose a reason for hiding this comment

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

This is pretty slick. 👍

serializer = CollectionVersionSerializer(page, many=True, context=context)
return self.get_paginated_response(serializer.data)

serializer = CollectionVersionSerializer(queryset, many=True, context=context)
Copy link
Member Author

Choose a reason for hiding this comment

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

the original endpoint for collections versions don't show all info we need for the unpaginated one:

{
   "meta":{
      "count":1
   },
   "links":{
      "first":"/api/automation-hub/v3/collections/vmware/vmware_rest/versions/?limit=10&offset=0",
      "previous":null,
      "next":null,
      "last":"/api/automation-hub/v3/collections/vmware/vmware_rest/versions/?limit=10&offset=0"
   },
   "data":[
      {
         "version":"1.0.1",
         "href":"/api/automation-hub/v3/collections/vmware/vmware_rest/versions/1.0.1/",
         "created_at":"2020-10-09T23:10:13.142203Z",
         "updated_at":"2020-10-09T23:10:13.142234Z"
      }
   ]
}

vs

[{
    "version": "string",
    "href": "string",
    "created_at": "2019-08-24T14:15:22Z",
    "updated_at": "2019-08-24T14:15:22Z",
    "artifact": "string",
    "collection": {
        "id": "string",
        "name": "string",
        "href": "string"
    },
    "download_url": "string",
    "name": "string",
    "namespace": {
        "name": "string"
    },
    "metadata": {
        "authors": [],
        "contents": {},
        "dependencies": {},
        "description": "string",
        "documentation": "string",
        "homepage": "string",
        "issues": "string",
        "license": [],
        "repository": "string",
        "tags": []
    }
},
...,
{
    "version": "string",
    "href": "string",
    "created_at": "2019-08-24T14:15:22Z",
    "updated_at": "2019-08-24T14:15:22Z",
    "artifact": "string",
    "collection": {
        "id": "string",
        "name": "string",
        "href": "string"
    },
    "download_url": "string",
    "name": "string",
    "namespace": {
        "name": "string"
    },
    "metadata": {
        "authors": [],
        "contents": {},
        "dependencies": {},
        "description": "string",
        "documentation": "string",
        "homepage": "string",
        "issues": "string",
        "license": [],
        "repository": "string",
        "tags": []
    }
}]

So I use the CollectionVersionSerializer instead of CollectionVersionListSerializer, and CollectionVersionSerializer needs the ContentArtifact to get artifact data

@@ -182,15 +182,15 @@ def get_artifact(self, obj):
"""
Get atrifact summary.
"""
return ArtifactRefSerializer(self.context["content_artifact"]).data
return ArtifactRefSerializer(obj.contentartifact_set.first()).data
Copy link
Member

Choose a reason for hiding this comment

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

Since this is replacing the removed line artifact = ContentArtifact.objects.get(content=instance) I think this could be

Suggested change
return ArtifactRefSerializer(obj.contentartifact_set.first()).data
return ArtifactRefSerializer(obj.contentartifact_set.get()).data


def get_download_url(self, obj) -> str:
"""
Get artifact download URL.
"""
host = settings.ANSIBLE_CONTENT_HOSTNAME.strip("/")
distro_base_path = self.context["path"]
filename_path = self.context["content_artifact"].relative_path.lstrip("/")
filename_path = obj.contentartifact_set.first().relative_path.lstrip("/")
Copy link
Member

Choose a reason for hiding this comment

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

Similar here to above ^

@@ -0,0 +1 @@
Introduce a new ``v3/metadata/collections/`` endpoint returning all collections unpaginated
Copy link
Member

Choose a reason for hiding this comment

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

nitpick:

Suggested change
Introduce a new ``v3/metadata/collections/`` endpoint returning all collections unpaginated
Introduces a new ``v3/metadata/collections/`` endpoint returning all collections unpaginated.

Comment on lines 1 to 2
Introduce a new ``v3/metadata/collection_versions/`` endpoint returning all collections versions
unpaginated
Copy link
Member

Choose a reason for hiding this comment

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

niptick:

Suggested change
Introduce a new ``v3/metadata/collection_versions/`` endpoint returning all collections versions
unpaginated
Introduces a new ``v3/metadata/collection_versions/`` endpoint returning all collections versions
unpaginated.

@bmbouter
Copy link
Member

So I've been testing this PR and it's working great. I'm strugglng to remember why we made two endpoints though.... @fao89 do you remember?

@fao89
Copy link
Member Author

fao89 commented Jan 27, 2021

So I've been testing this PR and it's working great. I'm strugglng to remember why we made two endpoints though.... @fao89 do you remember?

I moved the collection_versions endpoint to other PR: #509

@fao89
Copy link
Member Author

fao89 commented Jan 28, 2021

So I've been testing this PR and it's working great. I'm strugglng to remember why we made two endpoints though.... @fao89 do you remember?

I realize we need deprecation status, and collection_versions endpoint does not provide it

@fao89 fao89 requested a review from bmbouter January 28, 2021 17:31
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

Thank you!

@bmbouter bmbouter merged commit 6be64b8 into pulp:master Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants