The Wayback Machine - https://web.archive.org/web/20201031185441/https://github.com/pulp/pulp_docker/pull/334
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

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 ipanova:i4378 branch 2 times, most recently from f1a2a44 to ce0cc34 Apr 2, 2019
@ipanova
Copy link
Member Author

@ipanova 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)

This comment has been minimized.

@rochacbruno

rochacbruno Apr 2, 2019
Member

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 ipanova force-pushed the ipanova:i4378 branch from ce0cc34 to e0b6c37 Apr 3, 2019
@ipanova
Copy link
Member Author

@ipanova 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)

This comment has been minimized.

@asmacdo

asmacdo Apr 3, 2019
Contributor

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):

This comment has been minimized.

@asmacdo

asmacdo Apr 3, 2019
Contributor

I think this should probably also have these mixins: [list, read, delete]

This comment has been minimized.

@ipanova

ipanova Apr 3, 2019
Author Member

just create, this is a custom endpoint

endpoint_name = 'docker'
queryset = models.DockerPublisher.objects.all()
serializer_class = serializers.DockerPublisherSerializer
endpoint_name = 'docker/publications'

This comment has been minimized.

@asmacdo

asmacdo Apr 3, 2019
Contributor

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 ipanova:i4378 branch 2 times, most recently from a345360 to 6fcbffd Apr 3, 2019
@asmacdo
asmacdo approved these changes Apr 4, 2019
Copy link
Contributor

@asmacdo asmacdo left a comment

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

This comment has been minimized.

@asmacdo

asmacdo Apr 4, 2019
Contributor

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

@ipanova ipanova force-pushed the ipanova:i4378 branch from 6fcbffd to 61afd33 Apr 4, 2019
@ipanova ipanova merged commit 9a44d2e into pulp:master Apr 4, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.