On Tue, 2011-10-11 at 13:16 +0300, Alexander Bokovoy wrote:
> On Tue, 11 Oct 2011, Martin Kosek wrote:
> > On Tue, 2011-10-11 at 12:01 +0300, Alexander Bokovoy wrote:
> > > On Tue, 11 Oct 2011, Martin Kosek wrote:
> > > > @@ -212,6 +216,24 @@ class config_mod(LDAPUpdate):
> > > >                          raise errors.ValidationError(
> > > >                              name=k, error='attribute "%s" not allowed' 
> > > > % a
> > > >                          )
> > > Could you please also (in a separate patch) fix the above and others 
> > > by adding translations? Other exception messages in 
> > > plugins/config.py are designed to be used for user interactions but 
> > > this one is not localized.
> > 
> > Patch based on 142 for config plugin i18n fix attached. I created a
> ACK.
> 
> > ticket to fix all of these issues in 3.0 branch:
> > 
> > https://fedorahosted.org/freeipa/ticket/1953
> Thanks!
> 
>  
> > > 
> > > > +
> > > > +        for (attr, obj) in (('ipauserobjectclasses', 'user'),
> > > > +                            ('ipagroupobjectclasses', 'group')):
> > > > +            if attr in entry_attrs:
> > > > +                objectclasses = entry_attrs[attr] + 
> > > > self.api.Object[obj].possible_objectclasses
> > > would it make sense to do sort(set(objectclasses)) to get unique list 
> > > before using it further? Just a thought. get_allowed_attributes() will 
> > > go to LDAP's schema to consult about the attributes and it seems to me 
> > > we'd better not to do this multiple times for the same.
> > 
> > I added a list(set()) to remove duplicates, I don't think it is
> > necessary to sort it.
> Yes, this is fine.
> 
> > > > +                new_allowed_attrs = 
> > > > ldap.get_allowed_attributes(objectclasses,
> > > > +                                        raise_on_unknown=True)
> > > > +                checked_attrs = self.api.Object[obj].default_attributes
> > > > +                if self.api.Object[obj].uuid_attribute:
> > > > +                    
> > > > checked_attrs.append(self.api.Object[obj].uuid_attribute)
> > > > +                for obj_attr in 
> > > > self.api.Object[obj].default_attributes:
> > > Shouldn't this be checked_attrs?
> > > 
> > 
> > Correct. The previous version worked because .append modified the
> > original list. Fixed.
> Hm..
> +                checked_attrs = self.api.Object[obj].default_attributes
> doesn't change anything -- you still get a reference to 
> default_attributes and through 
>   checked_attrs += ....
> you'd modify the original one. Wouldn't the following be more correct:
> +                checked_attrs = copy(self.api.Object[obj].default_attributes)
> ?

This was done on purpose. When you combine 2 lists in Python using +
operator, a new list is created without modifying the old one. Check the
following example:

>>> a = [1,2,3]
>>> b = [4]
>>> c = a+b
>>> print c
[1, 2, 3, 4]
>>> print a
[1, 2, 3]
>>> print b
[4]
>>> c.append(5)
>>> print c
[1, 2, 3, 4, 5]
>>> print a
[1, 2, 3]
>>> print b
[4]

Martin

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to