Re: [Pulp-dev] Solving the "callback problem" ... aka how pulpcore will stop finalizing RepositoryVersion
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 wrote: > On Tue, Nov 5, 2019 at 5:01 PM Brian Bouterse wrote: > >> Thank you for writing! >> >> On Tue, Nov 5, 2019 at 4:29 PM Matthias Dellweg 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 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 >>> >
Re: [Pulp-dev] Solving the "callback problem" ... aka how pulpcore will stop finalizing RepositoryVersion
On Tue, Nov 5, 2019 at 5:01 PM Brian Bouterse wrote: > Thank you for writing! > > On Tue, Nov 5, 2019 at 4:29 PM Matthias Dellweg 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 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
Re: [Pulp-dev] Solving the "callback problem" ... aka how pulpcore will stop finalizing RepositoryVersion
On Tue, 5 Nov 2019 17:01:07 -0500 Brian Bouterse wrote: Comment below. > Thank you for writing! > > On Tue, Nov 5, 2019 at 4:29 PM Matthias Dellweg > 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 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
Re: [Pulp-dev] Solving the "callback problem" ... aka how pulpcore will stop finalizing RepositoryVersion
Thank you for writing! On Tue, Nov 5, 2019 at 4:29 PM Matthias Dellweg 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 > On Tue, 5 Nov 2019 14:46:18 -0500 > Brian Bouterse 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
Re: [Pulp-dev] Solving the "callback problem" ... aka how pulpcore will stop finalizing RepositoryVersion
On Tue, Nov 5, 2019 at 3:53 PM Simon Baatz wrote: > Hi Brian, > > On Tue, Nov 05, 2019 at 02:46:18PM -0500, Brian Bouterse wrote: > >... > >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. > >[3] > https://github.com/dralley/pulp_file/compare/typed-repositories...bm > >bouter: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. > > This is an elegant solution for the sync case and, as you said, > offers multiple options to implement validation/extra changes. > However there are other places in core that create repo versions. I > think we have three overall: sync, add_and_remove task, and create() > in SingleArtifactContentUploadSerializer. > > How will the plugin writer reuse validation code in the other places? > Currently, it seems that he/she has to duplicate some of the existing > core code to get the plugin's validation code into the right place. > Thank you for bringing this up! Here's one idea I had for the uploads use case: https://gist.github.com/bmbouter/81be284e6296c541ef9c4a0efc9b304f Note the it's not the same pattern because core's code is still performing the finalization, but the plugin writer is able to pass in a fully configured context manager so in that sense it's not a "hook". For the add_and_remove task, this change would be on top of typed repos. add_and_remove is called from only 1 place in @dralley's PR here: https://github.com/pulp/pulpcore/pull/354/files#diff-03cc41a4bcada47c767bb1615dc53cefR130 For that case we could decompose the 'modify' such that it could be used as-is if the plugin writer does not require the extra opportunity. If they do, then they could instantiate the plugin_context_manager in their viewset and pass it along to the task which could be modified like this: https://gist.github.com/bmbouter/0897648ce254faecbea730159f4e9cd1 Another option is to move @modify from pulpcore to the plugins themselves, but then plugins have to opt-in instead of opt-out. I expect instantiating the context manager in the viewset and passing the instance to the backend will work because RQ pickles it's job params: http://python-rq.org/docs/#on-the-design Note these gists aren't very "clean" but they should illustrate the idea. How about these approaches? ___ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
Re: [Pulp-dev] Solving the "callback problem" ... aka how pulpcore will stop finalizing RepositoryVersion
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. 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. On Tue, 5 Nov 2019 14:46:18 -0500 Brian Bouterse 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. pgpKGo7yXjuED.pgp Description: OpenPGP digital signature ___ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
Re: [Pulp-dev] Solving the "callback problem" ... aka how pulpcore will stop finalizing RepositoryVersion
Hi Brian, On Tue, Nov 05, 2019 at 02:46:18PM -0500, Brian Bouterse wrote: >... >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. >[3]https://github.com/dralley/pulp_file/compare/typed-repositories...bm >bouter: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. This is an elegant solution for the sync case and, as you said, offers multiple options to implement validation/extra changes. However there are other places in core that create repo versions. I think we have three overall: sync, add_and_remove task, and create() in SingleArtifactContentUploadSerializer. How will the plugin writer reuse validation code in the other places? Currently, it seems that he/she has to duplicate some of the existing core code to get the plugin's validation code into the right place. ___ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
[Pulp-dev] Solving the "callback problem" ... aka how pulpcore will stop finalizing RepositoryVersion
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
