On Tue, 5 Nov 2019 17:01:07 -0500 Brian Bouterse <bmbou...@redhat.com> wrote:
Comment below. > Thank you for writing! > > On Tue, Nov 5, 2019 at 4:29 PM Matthias Dellweg <dell...@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 <bmbou...@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.
pgpcTNkOi5LAZ.pgp
Description: OpenPGP digital signature
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
