#646: Add filters to select by related pulp_deb content types #647
Conversation
There was a problem hiding this comment.
Thanks for this submission. I think I understand what this feature is getting at. I cannot promise that we will get around to testing, reviewing, and merging this very soon, but we will try our best to do so by the next Y-release.
To make the linting turn green, you will need to squish all your commits into one and change the commit message similar to:
Add filters to select by related pulp_deb content types
closes #646
sdherr
force-pushed
the
component-filter
branch
from
6ada3c2
to
96710db
Compare
|
Thanks, I have squashed the commits and updated the commit message. |
|
@quba42 Ping, I believe this is ready for merging. We have been using this patch successfully for several months now. |
|
This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution! |
sdherr
force-pushed
the
component-filter
branch
from
96710db
to
9e51556
Compare
|
This pull request is no longer marked for closure. |
sdherr
force-pushed
the
component-filter
branch
from
9e51556
to
6803755
Compare
|
Reworked to ensure that the filter args are always hrefs, never uuids, and that a versionless repo href is acceptable too if you just want the latest version. |
There was a problem hiding this comment.
My apologies, but it looks like this change is rather badly affected by my refactor in #741 and by extension currently broken on latest main branch. It needs to be rebased and reworked. I already noted one needed change below, but more importantly ReleaseComponents no longer have a foreign key to Releases and are instead associated only loosely by both having the same distribution string (and there can be only one Release per distribution within a single repository version).
Right now I am getting django.core.exceptions.FieldError: Cannot resolve keyword 'deb_releasecomponent' into field. when filtering for Releases that have a particular package in a particular repo.
Querrying for release_components that have a particular package still works.
In the return direction filter for packages in a particular release or release_component I also get errors.
I am sorry I did not think of, and communicate this sooner. 😦
sdherr
force-pushed
the
component-filter
branch
2 times, most recently
from
0b9c4d5
to
e87ae29
Compare
|
@quba42 Right you are, I have updated the patch to work properly against the Release model changes. Here's an updated test scenario: |
|
I performed a round of testing with a real repository using: For the most part this all worked exactly like expected. If I remove the release from the repo, and then query for the release that contains my package, I get an empty list as expected. However, I can still querry for the release_component. However, if I querry for all the packages that are in the release (that is no longer in the latest repo version), I still find all the same packages as before which is a little odd. I will now go have a look at the code. |
There was a problem hiding this comment.
This looks mostly ready to me. Only found some minor issues below.
I will have another think about how we can/should add tests for this. I don't know to what extent we have any existing tests for filters, but perhaps some existing sync test can be easily extended to assert the filter finds the expected number of packages or the expected release.distribution etc. As we have seen during manual testing, it is definitely possible for some refactor to break this filter so some kind of coverage seems important.
|
I am going to leave another comment here, because I asked around An example of something similar in pulp_rpm: https://github.com/pulp/pulp_rpm/blob/main/pulp_rpm/tests/functional/api/test_copy.py#LL375C48-L375C48 Which means we should really figure out #734 since that is holding most of our existing sync tests hostage. |
@quba42 this sounds to me like an invalid repo state. I don't think it makes any sense to remove a I'm not against the idea of doing an additional check and throwing an error if the |
I support the idea of adding tests, at least simple "happy path" tests to ensure this is not broken by a future DB model change. But if this is ready to merge and the sync test PR is not then should we just add a note to that PR to add the tests? Or I'd be happy to circle back later when the testing situation is resolved and add something. Simple "happy path" tests similar to that example in |
There was a problem hiding this comment.
I performed another round of testing, and everything looks good to me.
I am fine with merging this as is, and working on a basic "happy path" test on a separate PR. That being said we did merge #734, so it should be possible to start working on a test. I will have another look what test could be relevantly extended.
|
Created a follow up task to add a test: #780 |
This aligns our pulp_deb patches with the changes that [have been requested upstream](pulp/pulp_deb#647) (namely, that we pass a full pulp href as the filter args instead of just uuid). The pulp_deb release filters have also been updated to assume the latest repo_version if you pass a repo href, so we can save ourselves a lookup here. You can look at the upstream diff to see a non-patch version of the first patch's changes (there is no upstream PR for the second patch): pulp/pulp_deb#647
These new filters allow you to ask the questions "What Packages are in the current version of this Release?" or "What ReleaseComponents contain this Package?" Examples:
It also provides a simple framework by which any similar types of filters can be added, and an explanation about why it works the way it does.