On Tue, Mar 27, 2018 at 12:02 AM, Austin Macdonald <aus...@redhat.com> wrote: > > > On Mon, Mar 26, 2018 at 4:43 PM, Dennis Kliban <dkli...@redhat.com> wrote: >> >> On Mon, Mar 26, 2018 at 3:38 PM, Austin Macdonald <aus...@redhat.com> >> wrote: >>> >>> >>> >>> On Mon, Mar 26, 2018 at 2:30 PM, Dennis Kliban <dkli...@redhat.com> >>> wrote: >>>> >>>> This proposal does not make the plugin writer's job any easier. This >>>> simply changes where the plugin writer is providing validation logic, in a >>>> serializer vs. in a view. >>> >>> >>> It does make the plugin writer's job easier *because* it changes how the >>> validation is provided, Plugin writers already have to understand how to >>> create models/serializers (and it is very simple) for all the other objects. >>> Seriallizer validation is much simpler than viewset validation because >>> serializers are literally designed to do this. >> >> >> If we don't change anything, a plugin writer is responsible for creating a >> task that creates a Repository Version and a view which will perform >> validation and then dispatches a task. If we do make the proposed change, >> the plugin writer will still have to write the task code and then also a >> serializer that performs validation. The proposed change only removes the >> responsibility of dispatching a task which is 1 line of code. > > > The difference is not in the number of lines, it is in the complexity. > Validation with serializers is simpler, and more similar to other plugin > code. > > New Way: > https://github.com/asmacdo/pulp_file/blob/8a8cbf81b6f0f1e382be7884f2b2406a06ea230b/pulp_file/app/serializers.py#L15-L21 > Old > way:https://github.com/asmacdo/pulp_file/blob/8a8cbf81b6f0f1e382be7884f2b2406a06ea230b/pulp_file/app/serializers.py#L13-L30 oh, and let's not forget the boilerplate needed to make the old way autodoc working for the custom action endpoints: https://github.com/pulp/pulp_file/blob/master/pulp_file/app/viewsets.py#L31,#L61 I dare to say majority of the sync objects (serializer) boilerplate will be generic enough to be shared between the plugins; there's not much one can do about a sync request but creating it...
> > That "1 line of code" puts the plugin writer in contact with a very complex > part of the pulpcore internals. `apply_async_with_reservation`: > https://github.com/pulp/pulp/blob/e8f804dc6595732dad38ec478877d919d7d97063/pulpcore/pulpcore/tasking/tasks.py#L180 > >>>> >>>> The other problem I see is that complexity for users is increased. >>>> Instead of having 1 resource for tracking task progress, there would be an >>>> infinite number of resources for tracking progress of tasks. >>> >>> >>> In the proposal, Tasks are Master/Detail. The user doesn't have to >>> "track" it at all. They can still use v3/tasks/ to see all the tasks. >>> Retrieving tasks will either be the same (v3/tasks/) or easier if they know >>> the task they are looking for (v3/tasks/file/syncs/) >> >> >> My understanding of the master/detail pattern we are using for our other >> resources is that each task's URI will look something like this: >> /api/v3/tasks/file/syncs/<uuid>/ >> >> If the above is true, that means the client has to be aware of >> '/api/v3/tasks/file/syncs/<uuid>/' resource. The user would need to know how >> to interpret the response from each task type. There could be an infinite >> number of these tasks with many different arguments. So writing client code >> handling all those responses could become a nightmare. Right now the user >> just needs to know how to interpret the response from '/api/v3/tasks/<uuid>' >> for all tasks. > > > I might be missing your point on this one. If you are talking about polling > tasks, the href of a new Task is returned when the Task is created.The > client should use the href that is returned, never string formatting urls. > Even so, the Tasks in this proposal will be part of the API schema, so > clients (and docs users) know where the endpoints are and what the arguments > should be. This would actually be a big advantage over the way things are > now. AFAIK, v3/importers/1234/sync/ is invisible to clients, so we replace > one invisible endpoint with one visible endpoint. No complexity added. > > If the client doesn't know how to interpret extra fields (the Task > parameters), they don't need to. All tasks share the same base fields, and > those are the fields that need to be interpreted when the client is polling > a task. +1 all tasks will share common ancestor and path in the url; it's very easy to explore and follow, see also the autodoc screen cast > >> >> Value #1: We have to do something to address the problem that >> adding/removing content to a repository without plugin input is incorrect. >> This proposal is one possibility, but it isn't valid to compare the value >> against doing nothing. Instead, if you don't like this option, we need to >> compare it to other proposals for how to involve plugins in tasks. >> >> >> We don't have a problem. I agree that a user needs to know a whole lot of >> information to correctly create a working repository version for a complex >> content type such as a Docker manifest. Without all this information on >> hand, the user could easily create a broken repository version. However, a >> rich client could solve that problem. If the plugin writer wants to support >> simpler clients, she can provide an additional URL for handling the >> validation and additional logic for creating a repository version. We should >> probably have a recommended convention for plugin authors to use when adding >> additional URLs. > > > What I find troubling is that the most basic action (add/remove) suddenly > becomes complex. If using the file plugin POST to > v3/repository/1234/versions/. If using docker, POST to > v3/plugins/docker/add/. This is especially irritating because the first way > will still work for docker, but will break corrupt the repositories. So the > responsibility falls to users to know that this endpoint "works" but should > be avoided (for certain plugins). It will be surprising that one is a "noun" > and the other is an "action". +1 this is a legit UX issue > > Also, doing correctness enforcement in the client instead of the REST API > seems very wrong. +1 > >> Value #5: Task history becomes much more useful. Currently, the only data >> on a task that connects it to user-facing objects is "created_resource". >> This proposal will allow users to filter tasks by parameters. For example, >> they can filter sync tasks by "repository" or "importer". They can also use >> the detail endpoints (v3/tasks/file/ or v3/tasks/file/syncs/) to list tasks >> based on the plugin or action. >> > >> I don't think tracking history is one of the use cases we decided to >> support in the MVP. We should have a separate discussion on how we could >> accomplish this. > > > Fair enough, but just because it isn't on the MVP doesn't mean this it > should be completely ignored. +1 Cheers, milan _______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev