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

Add PRN support #5813

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Add PRN support #5813

merged 1 commit into from
Sep 25, 2024

Conversation

gerrod3
Copy link
Contributor

@gerrod3 gerrod3 commented Sep 18, 2024

fixes: #5766

@gerrod3 gerrod3 force-pushed the prn-all branch 4 times, most recently from 463dd68 to af4fcce Compare September 23, 2024 15:45
@gerrod3 gerrod3 marked this pull request as ready for review September 23, 2024 21:26
@gerrod3 gerrod3 force-pushed the prn-all branch 2 times, most recently from e5bbdee to cf9fc98 Compare September 24, 2024 13:22
)
repo_versions[href_match["repository_pk"]].append(int(href_match["number"]))
if uri.startswith("prn:"):
model, pk = resolve_prn(uri)
Copy link
Member

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> ?

Copy link
Contributor Author

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
Copy link
Member

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?

Copy link
Contributor Author

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.

mdellweg
mdellweg previously approved these changes Sep 24, 2024
Copy link
Contributor

@ggainey ggainey left a 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"

pulpcore/plugin/util.py Show resolved Hide resolved
pulpcore/app/util.py Show resolved Hide resolved
Copy link
Contributor

@ggainey ggainey left a 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!

docs/dev/learn/subclassing/serializers.md Show resolved Hide resolved
docs/dev/learn/subclassing/serializers.md Show resolved Hide resolved
@gerrod3 gerrod3 force-pushed the prn-all branch 2 times, most recently from 5314e51 to ef97427 Compare September 24, 2024 16:58
docs/dev/learn/subclassing/serializers.md Show resolved Hide resolved
if name.endswith("Response"):
if "pulp_href" in schema["properties"]:
assert "prn" in schema["properties"], name
assert schema["properties"]["prn"]["type"] == "string", name
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

@ggainey ggainey Sep 24, 2024

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)

Copy link
Member

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.

@gerrod3 gerrod3 merged commit 0836506 into pulp:main Sep 25, 2024
12 checks passed
@gerrod3 gerrod3 deleted the prn-all branch September 25, 2024 19:25
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this pull request Oct 2, 2024
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
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this pull request Oct 2, 2024
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
dralley pushed a commit to pulp/pulp_rpm that referenced this pull request Oct 3, 2024
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
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this pull request Oct 7, 2024
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
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this pull request Oct 7, 2024
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
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this pull request Oct 7, 2024
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
dralley pushed a commit to pulp/pulp_rpm that referenced this pull request Oct 7, 2024
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
dralley pushed a commit to pulp/pulp_rpm that referenced this pull request Oct 7, 2024
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
dralley pushed a commit to pulp/pulp_rpm that referenced this pull request Oct 7, 2024
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
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.

Add PRN field to all resources in the REST API
4 participants