<div dir="ltr">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.<br clear="all"><div><div dir="ltr" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><br></div><div>David<br></div></div></div></div></div></div></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 6, 2019 at 9:22 AM Dennis Kliban <<a href="mailto:dkliban@redhat.com" target="_blank">dkliban@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Tue, Nov 5, 2019 at 5:01 PM Brian Bouterse <<a href="mailto:bmbouter@redhat.com" target="_blank">bmbouter@redhat.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Thank you for writing!<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 5, 2019 at 4:29 PM Matthias Dellweg <<a href="mailto:dellweg@atix.de" target="_blank">dellweg@atix.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Brian,<br>
i like the the change in the code flow, but since the<br>
DeclarativeVersion (in your example) does not create the repository<br>
version anymore, i think it should be renamed.<br>
Maybe it's the SyncPipeline and we call perform on it instead of create.<br></blockquote><div>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.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Also i do not see the benefit of making the RelativePathFixer a context<br>
manager instead of a simple function to be called after the pipeline.<br>
It can even go wrong badly, if __exit__ is called after an exeption<br>
broke the pipeline.<br></blockquote><div>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:  <a href="https://github.com/dralley/pulp_file/compare/typed-repositories...bmbouter:plugin-finalize-no-context-manager" target="_blank">https://github.com/dralley/pulp_file/compare/typed-repositories...bmbouter:plugin-finalize-no-context-manager</a></div><div> <br></div></div></div></blockquote><div><br></div>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. <br></div><div class="gmail_quote"><br></div><div class="gmail_quote">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. <br></div><div><br></div><div>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. <br></div><div><br></div><div>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. <br></div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
On Tue, 5 Nov 2019 14:46:18 -0500<br>
Brian Bouterse <<a href="mailto:bmbouter@redhat.com" target="_blank">bmbouter@redhat.com</a>> wrote:<br>
<br>
> As a followup to the chat discussion from triage/open-floor today,<br>
> here is the POC on top of typed repositories. It's actually a very<br>
> small change, the *only* significant difference is that the stages<br>
> API no longer uses the RepositoryVersion context manager. Thus, the<br>
> plugin writer must finalize it, but they can do that using<br>
> core-provided facilities. The links below are diffs on top of<br>
> @dalley's unmerged PRs so the links are long:<br>
> <br>
> <a href="https://github.com/dralley/pulpcore/compare/typed-repositories...bmbouter:plugin-context-manager" rel="noreferrer" target="_blank">https://github.com/dralley/pulpcore/compare/typed-repositories...bmbouter:plugin-context-manager</a><br>
> <a href="https://github.com/dralley/pulp_file/compare/typed-repositories...bmbouter:plugin-context-manager" rel="noreferrer" target="_blank">https://github.com/dralley/pulp_file/compare/typed-repositories...bmbouter:plugin-context-manager</a><br>
> <br>
> I'm able to run the pulp-smash test with these changes so I think it's<br>
> working:<br>
> django-admin test<br>
> pulp_file.tests.functional.api.test_sync.BasicFileSyncTestCase.test_sync<br>
> <br>
> Note that the context manager is only syntactic sugar. The pulp_file<br>
> sync code could also just as easily be as shown below. This is<br>
> incomplete, but I think you'll get the idea.<br>
> <br>
> <a href="https://github.com/dralley/pulp_file/compare/typed-repositories...bmbouter:plugin-finalize-no-context-manager" rel="noreferrer" target="_blank">https://github.com/dralley/pulp_file/compare/typed-repositories...bmbouter:plugin-finalize-no-context-manager</a><br>
> <br>
> With this plugins can even do what they want in terms of style<br>
> (context manager or not). Also they can not use it at all and the<br>
> only extra responsibility would be to finalize the RepositoryVersion<br>
> with its context manager (core provided).<br>
> <br>
> <br>
> I'd like to ask for feedback on this design asap. Questions are<br>
> concerns ... please send 'em.<br>
> <br>
> An extensive description was given at open floor, but those logs<br>
> aren't up yet. The gist is that content modification/validation will<br>
> require user options, the plugin already knows that, so let's stop<br>
> having the core finalize the RepositoryVersion.<br>
</blockquote></div></div>
_______________________________________________<br>
Pulp-dev mailing list<br>
<a href="mailto:Pulp-dev@redhat.com" target="_blank">Pulp-dev@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/mailman/listinfo/pulp-dev</a><br>
</blockquote></div></div>
_______________________________________________<br>
Pulp-dev mailing list<br>
<a href="mailto:Pulp-dev@redhat.com" target="_blank">Pulp-dev@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/mailman/listinfo/pulp-dev</a><br>
</blockquote></div>