On 06/10/2016 06:31 PM, Stanislav Laznicka wrote: > On 06/08/2016 02:06 PM, Florence Blanc-Renaud wrote: >> On 06/08/2016 10:07 AM, Petr Spacek wrote: >>> On 7.6.2016 15:11, Stanislav Laznicka wrote: >>>> Hello, >>>> >>>> Thank you for your patch. As the thin-client patches were pushed in the >>>> meantime, the patch won't apply. Could you please send a rebased version? >>>> >>>> Also, I have a few comments to the patch: >>>> >>>> 1) I think that the commit message should be rather a brief conclusion to >>>> the >>>> changes made in the commit. This could help for faster orientation in the >>>> changes that were made to a certain part of code should you be searching >>>> for a >>>> bug introduced by a commit. Should some more info be required, it can be >>>> added >>>> to the ticket. Could you therefore shorten the commit message? >>> (My personal opinion, no golden standard.) >>> >>> Honestly I disagree with Standa. Yes, the commit message seems to be a bit >>> long but *tickets* are not the best place to put *technical* information >>> into. >>> >>> Tickets are planning tool but keep in mind that Trac may/will vanish one day >>> and all we will have will be (Git?) repo. >>> >>> I would recommend putting the comment about expected input format into code >>> comments somewhere around batch command definition. >>> >>> This would reduce commit message (roughly, the text needs to be adapted) to >>> part starting 'The code did not check the format of ' ... which is perfectly >>> reasonable description of the change. >>> >>> IMHO. >>> Petr^2 Spacek >>> >>> >>>> 2) Please do not add the tickets to comments in the code. You can use git >>>> blame -L or git log -L to see in which commits were the changes introduced >>>> to >>>> a certain part of a file, these commits should include the ticket number if >>>> more info is needed. >>>> >>>> Standa >>>> >>>> >>>> On 05/27/2016 03:53 PM, Florence Blanc-Renaud wrote: >>>>> Hi all, >>>>> >>>>> the following patch checks the format of parameters passed to a method >>>>> called through the batch command. I picked the ConversionError for invalid >>>>> parameters format but this choice can be discussed if you have better >>>>> suggestions... >>>>> >>>>> Fixes:https://fedorahosted.org/freeipa/ticket/5810 >>>>> -- >>>>> Florence Blanc-Renaud >>>>> Identity Management Team, Red Hat >>>>> >>>>> >>>> >>>> >> Hi, >> >> please find an updated patch version with a less verbose commit msg. I also >> removed the reference to ticket # in the code. >> >> Flo. >> >> > Hello, > > Thank you for your updated patch. I have just one small issue that maybe > exceeds > the scope of this ticket. If the check for dictionary instance in list: > > + if not isinstance(arg, dict): > + raise errors.ConversionError( > + name='methods', > + error=_(u'must contain dict objects')) > > fails at a further member of the list, by raising an exception, you will lose > information about execution of all the previous commands but these were > already > executed. This hasn't seem to be an issue until now so I wonder if it is a > problem or not.
Right, this is something that we will need to address but not in scope of this ticket and probably not 4.4 release. > > So far, batch commands have only been utilized in the WebUI so I am adding > Petr > for opinions on how to handle this properly so that WebUI could react to it > should it ever happen (although AFAIK this should never happen for batch > commands called from the UI). Web UI doesn't care because it sends it correctly :) The bug is trying to use batch command while talking directly to API - e.g. because of performance reasons. > > Standa > -- Petr Vobornik -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
