On Fri, 21 Mar 2014 13:06:21 +0100 Petr Viktorin <[email protected]> wrote:
> On 03/21/2014 12:58 PM, Martin Kosek wrote: > > 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. > > Well, I think "DN of the added entry" is exactly what this is. > > > Also, "Task automember" label sounds strange to me, would > > "Automember task DN" be better? > > I meant "Task DN", sorry for the thinko. > > One more thing, which came to my mind after reviewing the code for myself once again, if the task fails with an exit code other than 0, there is a DatabaseError raised, which is just fine. But in the info, should I specify not only the exit code, but also the DN of the task, or is it unnecessary? Thanks Adam _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
