[Freeipa-devel] [PATCH] 0032 Cleanup when deleting a replica

[Simo Sorce]

On Mon, 20 Dec 2010 18:02:02 +0100
Jakub Hrozek <jhrozek at redhat.com> wrote:

> On Wed, Dec 15, 2010 at 08:01:10PM -0500, Simo Sorce wrote:
> > 
> > Clean up records related to the master being deleted in the shared
> > tree.
> > 
> > This also avoid issues later on if you want to rejoin the server as
> > a master. It is also needed in order to give back valid information
> > for patch 0035
> > 
> > Simo.
> > 
> > -- 
> > Simo Sorce * Red Hat, Inc * New York
> 
> >  def del_master(replman, hostname, force=False):
> > +    has_repl_agreement = True
> >      try:
> >          t = replman.get_agreement_type(hostname)
> >      except ldap.NO_SUCH_OBJECT:
> >          print "No replication agreement found for '%s'" % hostname
> > -        return
> > +        if force:
> > +            has_repl_agreement = False
> > +        else:
> > +            return
> >      except errors.NotFound:
> >          print "No replication agreement found for '%s'" % hostname
> > -        return
> > +        if force:
> > +            has_repl_agreement = False
> > +        else:
> > +            return
> 
> This is just a nitpick but the above except: blocks are exactly the
> same. One could remove the redundancy by just using:
>   
>   except (errors.NotFound, ldap.NO_SUCH_OBJECT):
> 
> > +
> > +    def replica_cleanup(self, replica, realm, force=False):
> > +
> > +        err = None
> > +
> > +        if replica == self.hostname:
> > +            raise RuntimeError("Can't cleanup self")
> > +
> > +        if not self.suffix or self.suffix == "":
> > +            self.suffix = util.realm_to_suffix(realm)
> > +            self.suffix = ipaldap.IPAdmin.normalizeDN(self.suffix)
> 
> This looks suspicious. Should one of these be in else: perhaps?

No, I just reused the same var to keep a temporary value, instead of
having a long line. not pretty but it is correct.
I can use a temp var if you think it makes for more readable code
though.

> The rest of the code looks OK, but I'm currently not able to test as
> the deletion fails with "Insufficient access". In my setup, vm-061 is
> the master and vm-038 is the replica:
> 
> [root at vm-061 ~]# ipa-replica-manage list vm-061.idm.lab.bos.redhat.com
> vm-038.idm.lab.bos.redhat.com
> [root at vm-061 ~]# ipa-replica-manage del vm-038.idm.lab.bos.redhat.com
> Unable to remove agreement on vm-038.idm.lab.bos.redhat.com:
> Insufficient access: 

Do you have a ticket as admin when you try this ?

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York