[Freeipa-devel] [PATCH][RFC] 7 automember rebuild nowait feature added

[Petr Viktorin]

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 <amisnyov at redhat.com> wrote:
>>
>>> On Fri, 14 Mar 2014 13:26:15 -0400
>>> Rob Crittenden <rcritten at redhat.com> 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']`.
>
> 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.


-- 
Petr³