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

[Petr Viktorin]

On 03/21/2014 12:00 PM, Misnyovszki Adam wrote:
> On Fri, 21 Mar 2014 10:33:00 +0100
> Petr Viktorin <pviktori at redhat.com> 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 <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']`.

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.)

-- 
Petr³