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

Problem: publication's distributions are full URLs #58

Merged
merged 1 commit into from
Apr 30, 2019

Conversation

dkliban
Copy link
Member

@dkliban dkliban commented Mar 28, 2019

Solution: use RelatedField instead of HyperlinkedRelatedField

The serializer was using the wrong type of field for serializing the Publication's distrubtions field.

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

@ipanova ipanova self-assigned this Apr 1, 2019
@ipanova
Copy link
Member

ipanova commented Apr 1, 2019

(pulp) [vagrant@pulp3-source-fedora29 pulp_docker]$ http GET  :8000/pulp/api/v3/publications/b519704c-09fd-4971-90ab-b32523bc26a4/
HTTP/1.1 200 OK
Allow: GET, DELETE, HEAD, OPTIONS
Connection: close
Content-Length: 318
Content-Type: application/json
Date: Mon, 01 Apr 2019 12:52:31 GMT
Server: gunicorn/19.9.0
Vary: Accept, Cookie
X-Frame-Options: SAMEORIGIN

{
    "_created": "2019-04-01T12:51:47.490564Z",
    "_distributions": [],
    "_href": "/pulp/api/v3/publications/b519704c-09fd-4971-90ab-b32523bc26a4/",
    "publisher": "/pulp/api/v3/publishers/rpm/rpm/7486be87-d367-40fa-9288-8ce9274df3ed/",
    "repository_version": "/pulp/api/v3/repositories/cb753815-2e6b-44f9-9e00-3361d010c584/versions/1/"
}

(pulp) [vagrant@pulp3-source-fedora29 pulp_docker]$ http POST http://localhost:8000/pulp/api/v3/distributions/ name='baz_zzz' base_path='foo_ddz' publication=/pulp/api/v3/publications/b519704c-09fd-4971-90ab-b32523bc26a4/
HTTP/1.1 202 Accepted
Allow: GET, POST, DELETE, HEAD, OPTIONS
Connection: close
Content-Length: 67
Content-Type: application/json
Date: Mon, 01 Apr 2019 12:52:46 GMT
Server: gunicorn/19.9.0
Vary: Accept, Cookie
X-Frame-Options: SAMEORIGIN

{
    "task": "/pulp/api/v3/tasks/cfe91146-6cc2-4f5a-9aee-7d09942627b9/"
}

(pulp) [vagrant@pulp3-source-fedora29 pulp_docker]$ http GET  :8000/pulp/api/v3/publications/b519704c-09fd-4971-90ab-b32523bc26a4/
HTTP/1.1 500 Internal Server Error
Connection: close
Content-Length: 27
Content-Type: text/html
Date: Mon, 01 Apr 2019 12:52:48 GMT
Server: gunicorn/19.9.0
Vary: Cookie
X-Frame-Options: SAMEORIGIN

<h1>Server Error (500)</h1>

(pulp) [vagrant@pulp3-source-fedora29 pulp_docker]$ http GET  :8000/pulp/api/v3/tasks/cfe91146-6cc2-4f5a-9aee-7d09942627b9/
HTTP/1.1 200 OK
Allow: GET, DELETE, HEAD, OPTIONS
Connection: close
Content-Length: 513
Content-Type: application/json
Date: Mon, 01 Apr 2019 12:52:54 GMT
Server: gunicorn/19.9.0
Vary: Accept, Cookie
X-Frame-Options: SAMEORIGIN

{
    "_created": "2019-04-01T12:52:46.056035Z",
    "_href": "/pulp/api/v3/tasks/cfe91146-6cc2-4f5a-9aee-7d09942627b9/",
    "created_resources": [
        "/pulp/api/v3/distributions/e82ca608-75ba-4983-b2c0-33fa6b3220ac/"
    ],
    "error": null,
    "finished_at": "2019-04-01T12:52:46.342781Z",
    "name": "pulpcore.app.tasks.distribution.create",
    "non_fatal_errors": [],
    "parent": null,
    "progress_reports": [],
    "spawned_tasks": [],
    "started_at": "2019-04-01T12:52:46.203275Z",
    "state": "completed",
    "worker": "/pulp/api/v3/workers/2371c737-f466-4d42-8e77-8bf658350788/"
}

(pulp) [vagrant@pulp3-source-fedora29 pulp_docker]$ 

@dkliban
Copy link
Member Author

dkliban commented Apr 5, 2019

Thanks for testing @ipanova. I reproduced the failure and fixed it.

@dkliban
Copy link
Member Author

dkliban commented Apr 5, 2019

i take it back. it looks like I can't specify BaseDistribution.objects.all() as the queryset. I will hold off on implementing a fix until after we make a decision on implementing this: https://pulp.plan.io/issues/4647

If that story is implemented then docker plugin will be able to define the '_distributions' field on it's own with the DockerDistribution.objects.all() as the queryset.

@ipanova ipanova removed their assignment Apr 24, 2019
@dkliban dkliban force-pushed the 4590 branch 3 times, most recently from fa68b1a to 61c8c30 Compare April 29, 2019 15:51
@dkliban
Copy link
Member Author

dkliban commented Apr 29, 2019

I've updated the PR. All the tests should be passing on Travis.

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.

This makes sense to me. Thanks @dkliban !

@codecov
Copy link

codecov bot commented Apr 29, 2019

Codecov Report

Merging #58 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
+ Coverage   67.82%   67.86%   +0.03%     
==========================================
  Files          65       65              
  Lines        3015     3015              
==========================================
+ Hits         2045     2046       +1     
+ Misses        970      969       -1
Impacted Files Coverage Δ
pulpcore/app/serializers/publication.py 98.88% <100%> (ø) ⬆️
pulpcore/app/viewsets/task.py 98.5% <0%> (+1.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b7f6c5...7aaddcc. Read the comment docs.

Solution: use RelatedField instead of HyperlinkedRelatedField

The serializer was using the wrong type of field for serializing the Publication's distrubtions field.

fixes: pulp#4590
https://pulp.plan.io/issues/4590
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

3 participants