On 11/02/2012 09:50 PM, Sumit Bose wrote: > On Fri, Nov 02, 2012 at 02:54:32PM +0100, Martin Kosek wrote: >> 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... >> > > fixed > >> >> 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" % \ >> >> > > fixed > > new version attached. > > bye, > Sumit >> Martin
NetBIOS name is now asked when first installing ipa-adtrust-install. But I see that NetBIOS name is still not queried when I run re-install of ADTRUST, I can only change current name via option. Is this is an intended behavior so that people cannot mess it with by mistake? # ipa-adtrust-install The log file for this installation can be found in /var/log/ipaserver-install.log ============================================================================== This program will setup components needed to establish trust to AD domains for the FreeIPA Server. This includes: * Configure Samba * Add trust related objects to FreeIPA LDAP server To accept the default shown in brackets, press the Enter key. IPA generated smb.conf detected. Overwrite smb.conf? [no]: y The following operations may take some minutes to complete. Please wait until the prompt is returned. <<< no NetBIOS name asked interactively Configuring cross-realm trusts for IPA server requires password for user 'admin'. This user is a regular system account used for IPA server administration. admin password: Configuring CIFS [1/18]: stopping smbd ... Otherwise the patch looks OK. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel