-
Notifications
You must be signed in to change notification settings - Fork 116
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
Add PRN support #5813
Add PRN support #5813
Conversation
463dd68
to
af4fcce
Compare
e5bbdee
to
cf9fc98
Compare
) | ||
repo_versions[href_match["repository_pk"]].append(int(href_match["number"])) | ||
if uri.startswith("prn:"): | ||
model, pk = resolve_prn(uri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So a repository version PRN is not prn:file.repository:<uuid>:<version>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, it is always prn:<app_label>.<model_label>:<uuid>
, even for repository versions.
assert ctx.value.status == 404 | ||
assert ctx.value.status == 400 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. Was it wrong raising 404 before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It kind of makes sense, you couldn't find a badly named resource so 404 not found. But it's an improper input so it should probably return 400, which it now does since I changed the filter code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to document, for plugin-authors, something along the lines of "if you subclass from ModelSerializer directly" (which plugins do) "you'll want to add 'prn' to your exposed-fields"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some docs-wordsmithing - otherwise, looking very good!
5314e51
to
ef97427
Compare
if name.endswith("Response"): | ||
if "pulp_href" in schema["properties"]: | ||
assert "prn" in schema["properties"], name | ||
assert schema["properties"]["prn"]["type"] == "string", name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails, right now, if you have pulp_rpm installed with "rpm.DistributionTreeResponse". I'm sure there are more. Can we set this up to collect/log all the failures, and then fail the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I didn't test with other plugins installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick "manual" scan shows pulp_rpm is missing the base fields in DistributionTreeSerializer
and UpdateCollectionSerializer
, fwiw. Would be good to have the test give us a complete list for a given install, so a run with many-plugins would ferret out the list of serializers that need to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've updated the test to group them together. Can you try it out and see if it is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha! OK, it works - but it still only finds DistributionTreeSerializer
- because UpdateCollectionSerializer
doesn't expose pulp_href
!
> assert len(failed) == 0
E AssertionError: assert 1 == 0
E + where 1 = len(['rpm.DistributionTreeResponse'])
Let me reset my env to include container and deb and let's see what we get, give me a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK cool, this looks like it does pick up all the ones it finds:
E AssertionError: assert 5 == 0
E + where 5 = len(['CollectionSummaryResponse', 'ansible.AnsibleDistributionResponse', 'ansible.AnsibleNamespaceMetadataResponse', 'ansible.CollectionVersionMarkResponse', 'rpm.DistributionTreeResponse'])
(That's from an env with DEV_SOURCE_PATH=pulpcore:pulp_rpm:pulp_container:pulp_python:pulp_deb:pulp_ansible
, btw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used to have a set of tests in pulpcore that will run on the plugins ci. I'm not sure this is still a thing. But this sounds like a good candidate.
fixes: pulp#5766
The prn field is not RPM metadata, it is pulp specific metadata, so this can be safely ignored. Prn was introduced in pulp/pulpcore#5813
The prn field is not RPM metadata, it is pulp specific metadata, so this can be safely ignored. Prn was introduced in pulp/pulpcore#5813
The prn field is not RPM metadata, it is pulp specific metadata, so this can be safely ignored. Prn was introduced in pulp/pulpcore#5813
The prn field is not RPM metadata, it is pulp specific metadata, so this can be safely ignored. Prn was introduced in pulp/pulpcore#5813
The prn field is not RPM metadata, it is pulp specific metadata, so this can be safely ignored. Prn was introduced in pulp/pulpcore#5813
The prn field is not RPM metadata, it is pulp specific metadata, so this can be safely ignored. Prn was introduced in pulp/pulpcore#5813
The prn field is not RPM metadata, it is pulp specific metadata, so this can be safely ignored. Prn was introduced in pulp/pulpcore#5813
The prn field is not RPM metadata, it is pulp specific metadata, so this can be safely ignored. Prn was introduced in pulp/pulpcore#5813
The prn field is not RPM metadata, it is pulp specific metadata, so this can be safely ignored. Prn was introduced in pulp/pulpcore#5813
fixes: #5766