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

Add a DownloadCatalog ViewSet #2987

Closed
wants to merge 1 commit into from

Conversation

fdobrovolny
Copy link
Contributor

@fdobrovolny fdobrovolny added the 3.0 label Apr 6, 2017
@mention-bot
Copy link

@BrnoPCmaniak, thanks for your PR! By analyzing the history of the files in this pull request, we identified @seandst, @goosemania and @asmacdo to be potential reviewers.

@fdobrovolny
Copy link
Contributor Author

@werwty Just letting you know I fixed the merge conflict.

@fdobrovolny fdobrovolny requested a review from werwty May 19, 2017 16:52
Copy link
Contributor

@werwty werwty left a comment

Choose a reason for hiding this comment

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

I tried running this PR as is, and got

ImproperlyConfigured at /api/v3/downloadcatalog/

Could not resolve URL for hyperlinked relationship using view name "artifact-details". You may have failed to include the related model in your API, or incorrectly configured the `lookup_field` attribute on this field.

There's two things in the catalog serializer:

  1. There is not going to be a Artifact ViewSet: https://pulp.plan.io/issues/2305#note-6
    This should be content instead
  2. view_name should be plural endpoint - singular detail. In this case view_name=importers-detail



class DownloadCatalogViewSet(NamedModelViewSet):
endpoint_name = 'downloadcatalog'
Copy link
Contributor

Choose a reason for hiding this comment

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

s/downloadcatalog/downloadcatalogs/
Endpoint names (the endpoint_name attribute) should plural, not singular [0]

from pulp.app.viewsets import NamedModelViewSet


class DownloadCatalogPagination(pagination.CursorPagination):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the pagination should be moved to here

@pep8speaks
Copy link

pep8speaks commented Aug 2, 2017

Hello @BrnoPCmaniak! Thanks for updating the PR.

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

Comment last updated on August 02, 2017 at 18:14 Hours UTC

Copy link
Contributor

@werwty werwty left a comment

Choose a reason for hiding this comment

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

The viewset looks good, there are some separate problems with the download catalog serializers, and I will file a redmine issue for that.

@jortel
Copy link
Contributor

jortel commented Aug 16, 2017

The DownloadCatalog has been replaced with the DeferredArtifact. Seems this work needs to be revised to be aligned with this.

This issue: 2968 removes the DownloadCatalog. Perhaps, this issue/pr could ensure that the DeferredArtifact model has a serializer and viewset?

[1] https://pulp.plan.io/issues/2968

@asmacdo
Copy link
Contributor

asmacdo commented Sep 26, 2017

Since this object has been replaced, I'm closing this issue.

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