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

Skip the publish and create directly the publication. #334

Merged
merged 1 commit into from
Apr 4, 2019

Conversation

ipanova
Copy link
Member

@ipanova ipanova commented Apr 2, 2019

@ipanova ipanova force-pushed the i4378 branch 2 times, most recently from f1a2a44 to ce0cc34 Compare April 2, 2019 14:40
@ipanova
Copy link
Member Author

ipanova commented Apr 2, 2019

@pulp/qe please also review pulp/pulp-smash#1194

@@ -77,21 +68,13 @@ def test_all(self):
non_latest = choice(version_hrefs[:-1])

# Step 2
publication = publish(self.cfg, publisher, repo)
publication1 = publication(self.cfg, DOCKER_PUBLICATION_PATH, repo)
Copy link
Member

Choose a reason for hiding this comment

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

Pulp-Smash now has a task_handler it was added recently in pulp/pulp-smash#1184

Now you don't need that extra publication function and can simply do

publication1 = self.client.using_handler(api.task_handler).post(
    DOCKER_PUBLICATION_PATH, 
    data={"repository": repo["_href"]}
)

The above post response will be passed to api.task_handler which will do exactly what your publication function is doing see: https://github.com/PulpQE/pulp-smash/blob/master/pulp_smash/api.py#L205-L268

@ipanova
Copy link
Member Author

ipanova commented Apr 3, 2019

@rochacbruno i updated the code and closed the opened PR, task_handler covers the usecase we need.

kwargs={
'publisher_pk': str(publisher.pk),
'repository_version_pk': str(repository_version.pk)
Copy link
Contributor

Choose a reason for hiding this comment

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

super duper nit: with the other line gone, it would be nice to put all the kwargs on a single line.

@@ -148,42 +149,40 @@ def sync(self, request, pk):
return OperationPostponedResponse(result, request)


class DockerPublisherViewSet(PublisherViewSet):
class DockerPublicationViewSet(NamedModelViewSet, mixins.CreateModelMixin):
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 this should probably also have these mixins: [list, read, delete]

Copy link
Member Author

Choose a reason for hiding this comment

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

just create, this is a custom endpoint

endpoint_name = 'docker'
queryset = models.DockerPublisher.objects.all()
serializer_class = serializers.DockerPublisherSerializer
endpoint_name = 'docker/publications'
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 this endpoint should be publications/docker. This is closer to the truth, the object is created is a publication, and the returned value of a /v3/publication will be less surprising.

Its worth noting that all the master/detail endpoints are structured in this way, ie content/docker remotes/docker

@ipanova ipanova force-pushed the i4378 branch 2 times, most recently from a345360 to 6fcbffd Compare April 4, 2019 16:58
Copy link
Contributor

@asmacdo asmacdo left a comment

Choose a reason for hiding this comment

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

I just noticed one potential surprise. I think this endpoint can publish any repository version, not just docker ones.

Hopefully people won't do that?

However we go, this is an improvement, +1 to merge

README.rst Outdated

Use the ``bar`` Publisher to create a Publication
Create a Publication
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the new endpoint name, I think we should call this "publish a repository version"

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.

3 participants