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

Dennis Kliban dkliban at redhat.com
Wed Nov 6 14:21:36 UTC 2019

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20191106/0f79292e/attachment.htm>

More information about the Pulp-dev mailing list