[Freeipa-devel] [PATCH] 157 Add --delattr option to complement --setattr/--addattr

[Martin Kosek]

On Mon, 2011-11-28 at 23:24 -0500, Rob Crittenden wrote:
> Martin Kosek wrote:
> > On Sat, 2011-11-05 at 13:43 +0100, Martin Kosek wrote:
> >> On Fri, 2011-11-04 at 09:23 -0400, Rob Crittenden wrote:
> >>> Martin Kosek wrote:
> >>>> Add a --delattr option to round out multi-valued attribute manipulation.
> >>>> The new option is be available for all LDAPUpdate based commands.
> >>>>
> >>>> --delattr is evaluated last, it can remove any value present either
> >>>> in --addattr/--setattr options or stored in LDAP.
> >>>>
> >>>> https://fedorahosted.org/freeipa/ticket/1929
> >>>
> >>> Should --delattr raise an error if the value doesn't exist?
> >>>
> >>> I think it probably should.
> >>>
> >>> rob
> >>
> >> You are right, it would be more expected behavior. I fixed that. In the
> >> process of implementing the change I found that current --*attr
> >> processing is not good, so I refactored it completely to one function
> >> available for all baseldap.py commands.
> >>
> >> In the process I found out that we don't have any common class for all
> >> baseldap.py commands and the result is BaseLDAPCommand which can now
> >> handle attribute processing and provide it to other LDAP commands.
> >>
> >> And I also fixed one group unit test. Now all unit tests were OK.
> >>
> >> Martin
> >
> > I rebased the patch (API.txt format was changed) and tested that it
> > still works ok.
> >
> > Martin
> 
> ACK but some of the comments need to be cleaned up before pushing. It 
> will also require a minor rebase in frontend.py.
> 
> process_attr_options() should probably read:
> 
> Process all --setattr, --addattr, and --delattr options and add the 
> resulting value to the list of attributes. --setattr is processed first,
> then --addattr and finally --delattr.
> 
> When --setattr is not used then the original LDAP object is looked up 
> (of course, not when dn is None) and the changes are applied to old 
> object values.
> 
> ...
> AttrValueNotFound exception may be raised when an attribute value was 
> not found either by --setattr and --addattr nor in existing LDAP object.
> 
> rob

I fixed the comment, it is now much more readable. I see you improved
__convert_2_dict function (which is moved to baseldap.py in this patch)
in your update plugin framework patch. This has been properly merged.

Pushed to master.

Martin