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

[Matthias Dellweg]

On Tue, 5 Nov 2019 17:01:07 -0500
Brian Bouterse <bmbouter at redhat.com> wrote:

Comment below.

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

I know, this is tedious, but i was reading the code and the naming
confused me completely. I think we could either call it
DeclarativeVersionContent or we would implement one SyncPipeline and
one MigrationPipeline anyway. Also is there any benefit in
instantiating that Pipeline object and calling `create` on it
separately? It sounds to me like a single
`populate_file_repository_version(new_repository_version, remote)`
followed by the optional constraints enforcer should do.

> > 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
> 
> 
> > 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.  
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20191106/e0522f0b/attachment.sig>