[Pulp-dev] Solving the "callback problem" ... aka how pulpcore will stop finalizing RepositoryVersion

[David Davis]

I've argued for removing the modify endpoint from core before we started
addressing how to enable validation and I think it's generally a good idea.
We already do this with other endpoints like sync so I don't think this
would be a new precedent. Also, some plugins (for whatever reason) may not
want to have a modify endpoint so defining it automatically for plugins
presents a problem in that case. I think it does create a tradeoff--one
that gives plugin writers more control but it also creates more work for
them.

David


On Wed, Nov 6, 2019 at 9:22 AM Dennis Kliban <dkliban at redhat.com> wrote:

> On Tue, Nov 5, 2019 at 5:01 PM Brian Bouterse <bmbouter at redhat.com> wrote:
>
>> Thank you for writing!
>>
>> On Tue, Nov 5, 2019 at 4:29 PM Matthias Dellweg <dellweg at atix.de> wrote:
>>
>>> Hi Brian,
>>> i like the the change in the code flow, but since the
>>> DeclarativeVersion (in your example) does not create the repository
>>> version anymore, i think it should be renamed.
>>> Maybe it's the SyncPipeline and we call perform on it instead of create.
>>>
>> I see what you're saying. What about the perspective that the
>> new_repository_version is the one DeclarativeVersion is acting upon? The
>> challenge w/ a name like SyncPipeline is that the Stages API aren't always
>> used for sync, for example the migration tool uses it for migrating
>> content. These are probably small semantic differences in the names, but
>> then again we're having a naming convo.
>>
>>
>>> Also i do not see the benefit of making the RelativePathFixer a context
>>> manager instead of a simple function to be called after the pipeline.
>>> It can even go wrong badly, if __exit__ is called after an exeption
>>> broke the pipeline.
>>>
>> I agree fully with your concerns and reasoning. I've shared examples w/
>> the "context_manager" approach, but since it's never involved in error
>> handling, let's plan that the actual implementation all over would be using
>> the style like this gist had:
>> https://github.com/dralley/pulp_file/compare/typed-repositories...bmbouter:plugin-finalize-no-context-manager
>>
>>
> The plugin writer's guide will recommend that every time the plugin writer
> creates a repository version they should call into some code that
> validates/fixes the new repository version. It's up to the plugin writer to
> keep this DRY. This sounds good to me.
>
> The 'modify' endpoint provided by core seems to be an exception to this
> rule though. I don't really see much of a difference between the plugin
> writer passing in a context manager or core calling into a plugin provided
> hook. I am not opposed to this pattern, but if we are trying to avoid it,
> then core most likely can't provide the repository version "modify"
> operation. In that case the rule from above applies to this operation also.
> The downside of that would be for the user who wouldn't be guaranteed to
> have this interface for every type of repository. That's ok with me. We can
> encourage plugin writers to implement certain interfaces.
>
> As for the SingleArtifactUploadSerializer, it can be treated the same way.
> The plugin writer has to implement their own 'create' method that performs
> the validation of the repository version.
>
> This all seems like a lot of work for plugin writers, but I just don't see
> how core can provide more without limiting what the plugin can provide.
>
>
>>
>>> On Tue, 5 Nov 2019 14:46:18 -0500
>>> Brian Bouterse <bmbouter at redhat.com> wrote:
>>>
>>> > As a followup to the chat discussion from triage/open-floor today,
>>> > here is the POC on top of typed repositories. It's actually a very
>>> > small change, the *only* significant difference is that the stages
>>> > API no longer uses the RepositoryVersion context manager. Thus, the
>>> > plugin writer must finalize it, but they can do that using
>>> > core-provided facilities. The links below are diffs on top of
>>> > @dalley's unmerged PRs so the links are long:
>>> >
>>> >
>>> https://github.com/dralley/pulpcore/compare/typed-repositories...bmbouter:plugin-context-manager
>>> >
>>> https://github.com/dralley/pulp_file/compare/typed-repositories...bmbouter:plugin-context-manager
>>> >
>>> > I'm able to run the pulp-smash test with these changes so I think it's
>>> > working:
>>> > django-admin test
>>> >
>>> pulp_file.tests.functional.api.test_sync.BasicFileSyncTestCase.test_sync
>>> >
>>> > Note that the context manager is only syntactic sugar. The pulp_file
>>> > sync code could also just as easily be as shown below. This is
>>> > incomplete, but I think you'll get the idea.
>>> >
>>> >
>>> https://github.com/dralley/pulp_file/compare/typed-repositories...bmbouter:plugin-finalize-no-context-manager
>>> >
>>> > With this plugins can even do what they want in terms of style
>>> > (context manager or not). Also they can not use it at all and the
>>> > only extra responsibility would be to finalize the RepositoryVersion
>>> > with its context manager (core provided).
>>> >
>>> >
>>> > I'd like to ask for feedback on this design asap. Questions are
>>> > concerns ... please send 'em.
>>> >
>>> > An extensive description was given at open floor, but those logs
>>> > aren't up yet. The gist is that content modification/validation will
>>> > require user options, the plugin already knows that, so let's stop
>>> > having the core finalize the RepositoryVersion.
>>>
>> _______________________________________________
>> 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/20191106/c03a5afc/attachment.htm>