[Pulp-dev] Repo version validation

Wed Oct 9 18:48:40 UTC 2019

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 <ttereshc at redhat.com>
wrote:

> On Wed, Oct 9, 2019 at 5:52 PM Brian Bouterse <bmbouter at redhat.com> wrote:
>
>>
>>
>> On Wed, Oct 9, 2019 at 5:23 AM Tatiana Tereshchenko <ttereshc at 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 <bmbouter at redhat.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Tue, Oct 8, 2019 at 2:55 PM David Davis <daviddavis at 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 <
>>>>> ttereshc at 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 <dkliban at 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 <daviddavis at 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 at redhat.com
>>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Pulp-dev mailing list
>>>>>>> Pulp-dev at redhat.com
>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>>>
>>>>>> _______________________________________________
>>>>> Pulp-dev mailing list
>>>>> Pulp-dev at redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20191009/2abf3be1/attachment.htm>