On 11/02/2012 12:54 PM, Sumit Bose wrote: > On Wed, Oct 31, 2012 at 04:03:14PM +0100, Martin Kosek wrote: >> On 10/30/2012 12:16 PM, Sumit Bose wrote: >>> Hi, >>> >>> this patch allows ipa-adtrust-install to reset the NetBIOS domain name >>> and fixes https://fedorahosted.org/freeipa/ticket/3192 . >>> >>> bye, >>> Sumit >>> >> >> >> Hello Sumit, >> >> I found few issues with your patch: > > Thank you for the review. > >> >> 1) It requires admin to be kinited ("conn.do_sasl_gssapi_bind()") I do not >> think this is necessary, ipa-adtrust-install already requires admin password >> to >> be passed and it already connects to LDAP with these credentials: >> >> api.Backend.ldap2.connect(ccache.name) >> >> You could use ipa.Backend.ldap2 object to do entry retrieval >> (ipa.Backend.ldap2.get_entry) without a need to init IPAdmin at all. > > fixed > >> >> 2) When doing try..except statement, rule of thumb says that it should be as >> short as possible, so that it does not hide other potential errors and makes >> clear what function raises the catched exception. >> >> In your case: >> >> try: >> entry = api.Backend.ldap2.get_entry(DN(('cn', api.env.domain), >> api.env.container_cifsdomains, >> self.api.env.basedn), >> ['ipantflatname']) >> except errors.NotFound: >> reset_netbios_name = False >> else: >> # process entry >> >> Should be a pattern that you want. > > fixed > > I also move all the NetBIOS name related code into a separate function. >> >> 3) I think this line is redundant: >> + print "Say 'yes' if the NetBIOS shall be changed and " \ >> + "'no' if the old one shall be kept." >> >> IMO, the question: >> >> reset_netbios_name = ipautil.user_input( 'Reset NetBIOS domain name?', >> default >> = False, allow_empty = False) >> >> and the information printed before is enough. > > I would prefer to keep it this way to make clear that > ipa-adtrust-install will continue processing, but the old name if kept > even if a new name was given with --netbios-name on the command line. > > New version attached. > > bye, > Sumit > >> >> Martin
The new approach looks much better. Sending issues I found with the new patch: 1) When I run ipa-adtrust-install on a clean IPA, I can no longer enter NetBIOS name interactively. I can only change it via script option... 2) I saw few typos: + print "Current NetBIOS domain name is %s new name is %s.\n" % \ should be: + print "Current NetBIOS domain name is %s, new name is %s.\n" % \ + print "NetBIOS domain name will be changes to %s.\n" % \ should be: + print "NetBIOS domain name will be changed to %s.\n" % \ Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
