Re: [Pulp-dev] Repo version validation
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 wrote: > On Wed, Oct 9, 2019 at 5:52 PM Brian Bouterse wrote: > >> >> >> On Wed, Oct 9, 2019 at 5:23 AM Tatiana Tereshchenko >> 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 >>> wrote: >>> On Tue, Oct 8, 2019 at 2:55 PM David Davis 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
Re: [Pulp-dev] Repo version validation
On Wed, Oct 9, 2019 at 5:52 PM Brian Bouterse wrote: > > > On Wed, Oct 9, 2019 at 5:23 AM Tatiana Tereshchenko > 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 >> wrote: >> >>> >>> >>> On Tue, Oct 8, 2019 at 2:55 PM David Davis >>> 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.
Re: [Pulp-dev] Repo version validation
On Wed, Oct 9, 2019 at 5:23 AM Tatiana Tereshchenko 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 > wrote: > >> >> >> On Tue, Oct 8, 2019 at 2:55 PM David Davis 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
Re: [Pulp-dev] Repo version validation
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. 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 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 wrote: > > > On Tue, Oct 8, 2019 at 2:55 PM David Davis 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 >> 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 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 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
Re: [Pulp-dev] Repo version validation
On Tue, Oct 8, 2019 at 2:55 PM David Davis 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 > 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 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 >>> 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
Re: [Pulp-dev] Repo version validation
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. David On Tue, Oct 8, 2019 at 12:55 PM Tatiana Tereshchenko 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 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 >> 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
Re: [Pulp-dev] Repo version validation
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 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] Repo version validation
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
