-
Notifications
You must be signed in to change notification settings - Fork 115
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
Enable filtering by content #2992
Conversation
Required PR: pulp/pulpcore#2992 [noissue]
Required PR: pulp/pulpcore#2992 [noissue]
Required PR: pulp/pulpcore#2992 [noissue]
9288ada
to
eeeb3ae
Compare
Required PR: pulp/pulpcore#2992 [noissue]
Required PR: pulp/pulpcore#2992 [noissue]
for dist in qs.exclude(repository=None).prefetch_related("repository__versions"): | ||
versions_distributions[dist.repository.latest_version().pk].append(dist.pk) |
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.
I think this doesn't handle the auto-distribute feature properly. For plugins that serve directly from a repository it'll work fine, but for plugins that use a publication it might be incorrect. When a repository is set on the distribution and that plugin uses publications, the distribution will serve the latest and highest repo-version publication. So technically you could have a repository with 10 versions, but only one publication on version 5 and that would be the version served. There is also a constant on the distribution class that is used to determine if serving from the publication is appropriate.
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.
Aha, I was not aware of that. Good to know.
eeeb3ae
to
8ee5667
Compare
Required PR: pulp/pulpcore#2992 [noissue]
Required PR: pulp/pulpcore#2992 [noissue]
Required PR: pulp/pulpcore#2992 [noissue]
8ee5667
to
315aeee
Compare
Required PR: pulp/pulpcore#2992 [noissue]
|
||
|
||
class DistributionWithContentFilter(Filter): | ||
"""A Filter class enabling filtering by content units exposed by distributions.""" |
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.
s/exposed/served
def __init__(self, *args, **kwargs): | ||
"""Initialize a help message for the filter.""" | ||
kwargs.setdefault( | ||
"help_text", _("Filter distributions based on the content present inside them") |
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.
based on the content served by them
@newswangerd please let us know what you think of this PR. Thank you! |
how do you use this? I assume it's |
315aeee
to
390720a
Compare
Required PR: pulp/pulpcore#2992 [noissue]
I have just realized that the old approach did not take into account distributions that do not serve content from publications. @newswangerd, would you mind testing your workflow once again? Now, it should work:
|
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.
Looks like it's working! Are you going to write functional tests for this?
Thanks! 🪂 There are functional tests written for this feature in pulp_file (pulp/pulp_file#778). I am still worried about the performance implications of doing so many queries under the hood. Would anyone from the @pulp/core team take a look at this PR? |
I looked at this PR. Yes this could probably be optimized more, but I generally recommend to write it simple first and then come back later after the runtime is demonstrated to be an issue in practice for our users. |
Required PR: pulp/pulpcore#2992 [noissue]
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 looks good to me. We can optimize later. Thanks!
@lubosmj what i am doing wrong?
|
@lubosmj i had old version of the PR, false alarm :) |
Required PR: pulp/pulpcore#2992 [noissue]
Required PR: pulp/pulpcore#2992 [noissue]
390720a
to
16b40d5
Compare
Required PR: pulp/pulpcore#2992 [noissue]
16b40d5
to
a5fd27a
Compare
Required PR: pulp/pulpcore#2992 [noissue]
closes #2952