Great, thank you for all the feedback! I posted a recap onto the issue here so we can get it ready for grooming: https://pulp.plan.io/issues/3541
Please send concerns or other thoughts/ideas. Thanks! Brian On Wed, Oct 9, 2019 at 2:22 PM Tatiana Tereshchenko <ttere...@redhat.com> wrote: > On Wed, Oct 9, 2019 at 5:52 PM Brian Bouterse <bmbou...@redhat.com> wrote: > >> >> >> On Wed, Oct 9, 2019 at 5:23 AM Tatiana Tereshchenko <ttere...@redhat.com> >> wrote: >> >>> I think the main confusion I have is that we call it validation. >>> Semantically, I'd expect the validation operation to complain if something >>> is invalid and to pass if everything is fine. >>> >> >> Yes let's have validation perform, just validation. Thank you for >> bringing us back to this. +1 >> >> >>> The solution [0] also implies that I think: >>> Raises: >>> django.core.exceptions.ValidationError: if the repository is >>> invalid >>> >>> As I mentioned below I think a plugin needs to have an ability to change >>> the content of a repo version, it sounds more than just validation to me. >>> Let me know if I misunderstand something or misuse any terms. >>> >> I agree. What about if we add add_content, remove_content classmethod for >> plugin writers to override on their Content subclass, and these were called >> by RepositoryVersion.add_content and RepositoryVersion.remove_content? >> These would be hooks similar to the validation hook, only these are >> designed to "handle" changes. The queryset would be type-filtered in the >> RepositoryVersion.add_content and RepositoryVersion.remove_content, so each >> Content model's implementation only needs to handle it's own type and it >> can rely on that. I'm open to other pattern suggestions with a similar >> outcome. >> > > +1 to this idea. Thank you! > As we discussed offline, let's additionally pass a repo version itself > to add_content/remove_content plugin classmethod because for some content > types it's necessary to have access to other types in a repo version. E.g. > modulemd and its RPMs. > > >> >> This would handle the Advisory-merge use case for pulp_rpm for example. >> The Advisory object would define these two hook implementations, and when >> adding it would provide the merge-with-existing-content feature. >> >> We may need to take care that references to content could be invalidated >> anytime a call to RepositoryVersion.add_content or >> RepositoryVersion.remove_content occurs. That sounds do-able but it becomes >> a somewhat subtle requirement. >> > > +1 to the concern > Additionally, new content can be added during those calls. E.g. Advisory > merge = removal of old one, addition of newly created one. > > >> >> I don't think this would fully handle the dependency use case though. >> This type of pattern only can account for the content that is already in >> the repsitory_version being created. So it lacks a reference to for example >> where content is being copied from, and what source content set it should >> look into to even provide dependency resolution. Also at sync-time it >> couldn't know if the packages it's "going to bring in extra" are just >> coming in a later part of the pipeline. So while I think we should >> implement these hooks, it doesn't solve the dependency solving use case. >> I'm ok w/ that, but what do you think? >> > > I think it's fine. Dependency solving is a pure copy case, in my opinion, > so I'm not concerned that we don't cover it in the generic way for every > repo version. > > Tanya > > >> >> >>> I keep thinking these use cases are for copy not sync, because only in >>>> the copy case is the plugin writer's code not already involved. >>> >>> I think any use case that modifies repo version in some way is important >>> here - sync, copy, upload, removal of content. >>> It's just happened that for sync we already have a mechanism for plugins >>> to influence the result, however it can likely be simplified and reuse what >>> will be implemented for the story [0] under discussion. >>> >>> Tanya >>> >>> [0] https://pulp.plan.io/issues/3541#note-3 >>> >>> On Tue, Oct 8, 2019 at 11:01 PM Brian Bouterse <bmbou...@redhat.com> >>> wrote: >>> >>>> >>>> >>>> On Tue, Oct 8, 2019 at 2:55 PM David Davis <davidda...@redhat.com> >>>> wrote: >>>> >>>>> I think @bmbouter's solution could handle the first example of >>>>> checking RPMs against a specific key. >>>>> >>>>> The second example is trickier though because the validation would >>>>> have to know which module is being removed in order to know which packages >>>>> to remove from the repo. This is because the packages could exist in the >>>>> repo independently of the module. I think it'd have to have the list of >>>>> additions/removals in order to handle that use case. >>>>> >>>> >>>> It would have reference to the repo_version being created, so I think >>>> it would have the RepositoryVersion.removed queryset to inspect. I think >>>> this is mainly useful for copy operations at which point the copy endpoint >>>> may be a better tool for features like plugin-provided dependency >>>> resolution versus the generic copy operations in core. >>>> >>>> I keep thinking these use cases are for copy not sync, because only in >>>> the copy case is the plugin writer's code not already involved. >>>> >>>> >>>>> David >>>>> >>>>> >>>>> On Tue, Oct 8, 2019 at 12:55 PM Tatiana Tereshchenko < >>>>> ttere...@redhat.com> wrote: >>>>> >>>>>> The solution proposed in #3541 looks good for validation purposes. >>>>>> My understanding of the problem is that a plugin needs to apply some >>>>>> logic and decide which content to keep and which content to remove at >>>>>> repo >>>>>> version creation time and perform these changes. >>>>>> Examples: >>>>>> - add to a repo version only RPMs signed with a specific key >>>>>> - removal of the moduled content should automagically remove related >>>>>> RPMs from a repo version. >>>>>> >>>>>> In theory, for the examples above, if we have validation only, user >>>>>> can be forced to prepare perfect add/remove requests, however I think it >>>>>> won't be a good user experience. >>>>>> >>>>>> Can it be done in the same way as the suggestion for validation? Just >>>>>> if it makes sense for plugin to "fix" repo version itself, they will do >>>>>> it, >>>>>> otherwise validation error can be raised. What do you think? >>>>>> >>>>>> Tanya >>>>>> >>>>>> >>>>>> On Tue, Oct 8, 2019 at 4:46 PM Dennis Kliban <dkli...@redhat.com> >>>>>> wrote: >>>>>> >>>>>>> The plan outlined in 3541 solves the problem in a way that gives >>>>>>> plugin writers a lot of control. +1 to implementing it. >>>>>>> >>>>>>> On Thu, Oct 3, 2019 at 12:23 PM David Davis <davidda...@redhat.com> >>>>>>> wrote: >>>>>>> >>>>>>>> We have a blocker for Pulp 3.0 GA[0] that we need to address soon >>>>>>>> in order to let plugins leverage it in their upcoming GA releases. It >>>>>>>> involves allowing plugin writers to validate content in a repo version. >>>>>>>> It's somewhat related to validating uniqueness in a repo version[1] >>>>>>>> except >>>>>>>> there are cases other than uniqueness that plugins might want to >>>>>>>> handle. >>>>>>>> One example might be a case where we want to prevent a user from >>>>>>>> adding a >>>>>>>> docker tag that points to a manifest outside a repo from getting added >>>>>>>> to >>>>>>>> the repo. I'm not sure if this is an actual example but it gives you an >>>>>>>> idea that there might be other non-unique validation plugin writers >>>>>>>> might >>>>>>>> want to add. >>>>>>>> >>>>>>>> Brian proposed a solution on 3541 that I think solves the >>>>>>>> problem[2]. I was hoping to maybe get some feedback on it so we could >>>>>>>> proceed by October 9. >>>>>>>> >>>>>>>> [0] https://pulp.plan.io/issues/3541 >>>>>>>> [1] https://pulp.plan.io/issues/5008 >>>>>>>> [2] https://pulp.plan.io/issues/3541 >>>>>>>> >>>>>>>> David >>>>>>>> _______________________________________________ >>>>>>>> Pulp-dev mailing list >>>>>>>> Pulp-dev@redhat.com >>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> Pulp-dev mailing list >>>>>>> Pulp-dev@redhat.com >>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>> >>>>>> _______________________________________________ >>>>> Pulp-dev mailing list >>>>> Pulp-dev@redhat.com >>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>> >>>>
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
