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