On 03/21/2014 12:38 PM, Petr Viktorin wrote: > On 03/21/2014 12:00 PM, Misnyovszki Adam wrote: >> On Fri, 21 Mar 2014 10:33:00 +0100 >> Petr Viktorin <[email protected]> wrote: >> >>> On 03/21/2014 10:29 AM, Petr Viktorin wrote: >>>> On 03/20/2014 04:22 PM, Misnyovszki Adam wrote: >>>>> On Thu, 20 Mar 2014 14:19:51 +0100 >>>>> Misnyovszki Adam <[email protected]> wrote: >>>>> >>>>>> On Fri, 14 Mar 2014 13:26:15 -0400 >>>>>> Rob Crittenden <[email protected]> wrote: >>>>>> >>>>>>> Misnyovszki Adam wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> automember-rebuild uses asynchronous 389 task, and returned >>>>>>>> success even if the task didn't run. This patch fixes this issue >>>>>>>> adding a --nowait parameter to 'ipa automember-rebuild', >>>>>>>> defaulting to False, thus when the script runs without it, it >>>>>>>> waits for the 'nstaskexitcode' attribute, which means the task >>>>>>>> has finished, according to >>>>>>>> http://directory.fedoraproject.org/wiki/Task_Invocation_Via_LDAP#Implementation. >>>>>>>> >>>>>>>> >>>>>>>> Old usage can be enabled using --nowait. >>>>>>>> >>>>>>>> https://fedorahosted.org/freeipa/ticket/4239 >>>>>>>> >>>>>>>> Request for comments: >>>>>>>> - Should I add a parameter to specify the polling time? (now >>>>>>>> 1ms) >>>>>>>> - Should I add a parameter to specify the maximum polling >>>>>>>> number? Now if something fails about creating the task, it >>>>>>>> polls forever. >>>>>>>> - Obviously, if these parameters should be added, there should >>>>>>>> be a reasonable default for them (~ Required=False, Default=X). >>>>>>> >>>>>>> I don't think you need a polling time, esp since this is hidden >>>>>>> from the user, but I think that is probably too short and you >>>>>>> may end up hammering the LDAP server. >>>>>>> >>>>>>> I also wonder if there should be some maximum wait time. I don't >>>>>>> like loops that can never exit. I'm at a loss for what that time >>>>>>> should be though. And we'd need to spell out that we gave up >>>>>>> waiting, not that the task necessarily failed. So rather than >>>>>>> having a polling time option, rename nowait into wait_for=20, so >>>>>>> wait for 20 seconds. Or something like that. >>>>>>> >>>>>>> I'd suggest using get_entry since you already know the full DN >>>>>>> and there is only ever one. It would make this much more >>>>>>> readable. >>>>>>> >>>>>>> I wonder if some top-level documentation should be added to flesh >>>>>>> this out some more. This does, for example, return False in one >>>>>>> case. The meaning for that should be spelled out. >>>>>>> >>>>>>> rob >>>>>> >>>>>> Hi, >>>>>> personally I would keep --no-wait, with a delay of 1 seconds, and >>>>>> a maximum waiting time for 60 seconds. Questions is, do we need to >>>>>> parameterize with other parameters(wait-for to the maximum time, >>>>>> and/or poll-delay for the delay time, both not required), or it is >>>>>> not a case which worth to develop? >>>>>> Greets >>>>>> Adam >>>>> >>>>> Hi, >>>>> here are the corrections Petr asked, also the other patch conatins >>>>> the plugin registration refactor. >>>> >>>> >>>> Thanks! >>>> >>>> You forgot an alternate summary for the --no-wait case. (Make sure >>>> to output the DN in this case, so it's possible to poll for it.) >>>> >>>> >>>> >>>> Instead of `task['nstaskexitcode'][0]` please use >>>> `task.single_value['nstaskexitcode']`. > > There's one more `task['nstaskexitcode'][0]`. (Perhaps putting it in a > variable > would be better?) > >>>> >>>> Here: >>>> >>>> raise errors.DatabaseError( >>>> desc=_("Automember rebuild membership task failed"), >>>> info=_("nstaskexitcode = '%s'" % >>>> str(task['nstaskexitcode'][0]))) >>>> >>>> there's no need to call str() on %'s argument. >>>> Also, use natural language (like "Task exit code: %s"), otherwise >>>> there's no need to mark the message for translation. >>>> >>>> >>> >>> And one more thing: Please bump the minor version in the VERSION file >>> when API.txt changes. >>> >>> >> >> Hi, >> everything is corrected! >> Greets >> Adam >> > > Sorry for dragging this so long, but: > The DN should not be a part of the summary, because that makes it hard to > parse > when using the API. It should be returned as a part of the result. > To do that, you'd need to change the output type: > > has_output = output.standard_entry > has_output_params = ( > DNParam( > 'dn', > label=_('Task automember'), > doc=_('DN of the started task'), > ), > ) > > and provide a dict in result, instead of True. (And with --no-wait, add the dn > to that dict.) >
Do really want to use 'dn' for the DNParam? dn is already used for standard entry DN when --all is used, right? "automembertaskdn" may be better. Also, "Task automember" label sounds strange to me, would "Automember task DN" be better? Martin _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
