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 <dkli...@redhat.com> wrote: > On Tue, Nov 5, 2019 at 5:01 PM Brian Bouterse <bmbou...@redhat.com> wrote: > >> 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. >> >> >>> 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 <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. >>> >> _______________________________________________ >> Pulp-dev mailing list >> Pulp-dev@redhat.com >> https://www.redhat.com/mailman/listinfo/pulp-dev >> > _______________________________________________ > Pulp-dev mailing list > Pulp-dev@redhat.com > https://www.redhat.com/mailman/listinfo/pulp-dev >
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev