On Fri, 21 Mar 2014 13:06:21 +0100
Petr Viktorin <pvikt...@redhat.com> 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 <pvikt...@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 <amisn...@redhat.com> wrote:
> >>>>>>
> >>>>>>> On Fri, 14 Mar 2014 13:26:15 -0400
> >>>>>>> Rob Crittenden <rcrit...@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.)
> >>
> >
> > 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
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to