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 <amisn...@redhat.com> wrote:

On Fri, 14 Mar 2014 13:26:15 -0400
Rob Crittenden <rcrit...@redhat.com> wrote:

Misnyovszki Adam wrote:

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

Old usage can be enabled using --nowait.


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.


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?

here are the corrections Petr asked, also the other patch conatins the
plugin registration refactor.


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


    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.


Freeipa-devel mailing list

Reply via email to