Thanks Brian. I still see few issues though:

1) The patch adds a whitespace error:

$ git apply ~/freeipa-bcook-0001-Add-DNS-Setup-Prompt-to-Install.patch
/home/mkosek/freeipa-bcook-0001-Add-DNS-Setup-Prompt-to-Install.patch:41:
trailing whitespace.

warning: 1 line adds whitespace errors

2) It does unrelated and unnecessary changes to the main function:

--- a/install/tools/ipa-server-install
+++ b/install/tools/ipa-server-install
@@ -560,10 +560,16 @@ def set_subject_in_config(realm_name, dm_password,
suffix, subject_base):
         conn.disconnect()

 def main():
+    """
+
+
+    :return:
+    """
     global ds
     global pw_name
     global uninstalling
     global installation_cleanup
+
     ds = None

     safe_options, options = parse_options()

3) In the question, I would replace "bind" with "BIND" as this is how the
project should be spelled. I see that few lines above we also refer to BIND
with "bind" (it may have caused the confusion), I think this can be fixed too.

Martin


On 02/15/2013 03:07 AM, Brian Cook wrote:
> Thanks Martin and Dmitri.  I have attached a patch that I -think- is formatted
> correctly... I removed the new variable and added check for --unattended.
> 
> Thanks,
> Brian 
> 
> 
> -------------------------------------------------------------------------------
> *From: *"Martin Kosek" <mko...@redhat.com>
> *To: *d...@redhat.com
> *Cc: *freeipa-devel@redhat.com
> *Sent: *Wednesday, February 13, 2013 11:16:51 PM
> *Subject: *Re: [Freeipa-devel] patch for trac 2575
> 
> On 02/14/2013 03:49 AM, Dmitri Pal wrote:
>> On 02/13/2013 05:20 PM, Brian Cook wrote:
>>> Please disregard the first patch as it still asked the user if they want to
> install DNS even if --setup-dns was passed, this one is fixed.
>>>
>>> Brian
>>
>> Brian,
>>
>> Thanks for the patch.
>> Can you please format it following these guidelines:
>> https://fedorahosted.org/freeipa/wiki/PatchFormat
>>
>> Thanks
>> Dmitri
> 
> Hello Brian,
> 
> Thanks for the patch! Also few technical notes:
> 
> 1) There is no need to invent the new variable, you can ask and set
> options.setup_dns to True. We already to this in other parts incode
> 
> 2) This patch would --unattended mode when no --setup-dns is passed
> 
> Martin
> 
>>>
>>>
>>>
>>> diff --git a/install/tools/ipa-server-install 
>>> b/install/tools/ipa-server-install
>>> index 1559107..96ef802 100755
>>> --- a/install/tools/ipa-server-install
>>> +++ b/install/tools/ipa-server-install
>>> @@ -564,6 +564,7 @@ def main():
>>>      global pw_name
>>>      global uninstalling
>>>      global installation_cleanup
>>> +    
>>>      ds = None
>>>  
>>>      safe_options, options = parse_options()
>>> @@ -740,8 +741,18 @@ def main():
>>>      admin_password = ""
>>>      reverse_zone = None
>>>  
>>> -    # check bind packages are installed
>>> +    # Setup a variable to use instead of options.setup_dns to enable
> interactive DNS selection
>>> +    setup_dns=False
>>>      if options.setup_dns:
>>> +        setup_dns=True
>>> +    else:
>>> +    # Ask user if they want to install DNS    
>>> +        if ipautil.user_input("Do you want to configure integrated DNS
> (bind)?", False):
>>> +            setup_dns=True
>>> +
>>> +
>>> +    # check bind packages are installed
>>> +    if setup_dns:
>>>          if not bindinstance.check_inst(options.unattended):
>>>              sys.exit("Aborting installation")
>>>  
>>> @@ -827,7 +838,7 @@ def main():
>>>      else:
>>>          admin_password = options.admin_password
>>>  
>>> -    if options.setup_dns:
>>> +    if setup_dns:
>>>          if options.no_forwarders:
>>>              dns_forwarders = ()
>>>          elif options.forwarders:
>>> @@ -858,7 +869,7 @@ def main():
>>>      print "Realm name:    %s" % realm_name
>>>      print
>>>  
>>> -    if options.setup_dns:
>>> +    if setup_dns:
>>>          print "BIND DNS server will be configured to serve IPA domain 
>>> with:"
>>>          print "Forwarders:    %s" % ("No forwarders" if not dns_forwarders 
>>> \
>>>                  else ", ".join([str(ip) for ip in dns_forwarders]))
>>> @@ -1102,7 +1113,7 @@ def main():
>>>                 persistent_search=options.persistent_search,
>>>                 serial_autoincrement=options.serial_autoincrement,
>>>                 ca_configured=not options.selfsign)
>>> -    if options.setup_dns:
>>> +    if setup_dns:
>>>          api.Backend.ldap2.connect(bind_dn=DN(('cn', 'Directory Manager')),
> bind_pw=dm_password)
>>>  
>>>          bind.create_instance()
>>> @@ -1147,11 +1158,11 @@ def main():
>>>      print "\t\t  * 80, 443: HTTP/HTTPS"
>>>      print "\t\t  * 389, 636: LDAP/LDAPS"
>>>      print "\t\t  * 88, 464: kerberos"
>>> -    if options.setup_dns:
>>> +    if setup_dns:
>>>          print "\t\t  * 53: bind"
>>>      print "\t\tUDP Ports:"
>>>      print "\t\t  * 88, 464: kerberos"
>>> -    if options.setup_dns:
>>> +    if setup_dns:
>>>          print "\t\t  * 53: bind"
>>>      if options.conf_ntp:
>>>          print "\t\t  * 123: ntp"
>>>
>>>
>>>
>>>
>>>> Message: 8
>>>> Date: Wed, 13 Feb 2013 13:39:32 -0800
>>>> From: Brian Cook <bc...@redhat.com>
>>>> To: "freeipa-devel@redhat.com" <freeipa-devel@redhat.com>
>>>> Subject: [Freeipa-devel] patch for trac 2575
>>>> Message-ID: <9dd1d1bb-6b86-4ea1-b61b-b208e6bc7...@redhat.com>
>>>> Content-Type: text/plain; charset="windows-1252"
>>>>
>>>> This is a patch for ticket 2575 on trac: [RFE] Installer wizard should
> prompt for DNS.  This is my first time submitting a patch so I was looking for
> something that seemed relatively easy?
>>>>
>>>> Thanks,
>>>> Brian
>>>>
>>>>
>>>> diff --git a/install/tools/ipa-server-install
> b/install/tools/ipa-server-install
>>>> index 1559107..d8c4ae5 100755
>>>> --- a/install/tools/ipa-server-install
>>>> +++ b/install/tools/ipa-server-install
>>>> @@ -564,6 +564,7 @@ def main():
>>>>     global pw_name
>>>>     global uninstalling
>>>>     global installation_cleanup
>>>> +    
>>>>     ds = None
>>>>
>>>>     safe_options, options = parse_options()
>>>> @@ -740,8 +741,18 @@ def main():
>>>>     admin_password = ""
>>>>     reverse_zone = None
>>>>
>>>> -    # check bind packages are installed
>>>> +    # Setup a variable to use instead of options.setup_dns to enable
> interactive DNS selection
>>>> +    setup_dns=False
>>>>     if options.setup_dns:
>>>> +        setup_dns=True
>>>> +    
>>>> +    # Ask user if they want to install DNS    
>>>> +    if ipautil.user_input("Do you want to cnfigure integrated DNS
> (bind)?", false):
>>>> +        setup_dns=True
>>>> +
>>>> +
>>>> +    # check bind packages are installed
>>>> +    if setup_dns:
>>>>         if not bindinstance.check_inst(options.unattended):
>>>>             sys.exit("Aborting installation")
>>>>
>>>> @@ -827,7 +838,7 @@ def main():
>>>>     else:
>>>>         admin_password = options.admin_password
>>>>
>>>> -    if options.setup_dns:
>>>> +    if setup_dns:
>>>>         if options.no_forwarders:
>>>>             dns_forwarders = ()
>>>>         elif options.forwarders:
>>>> @@ -858,7 +869,7 @@ def main():
>>>>     print "Realm name:    %s" % realm_name
>>>>     print
>>>>
>>>> -    if options.setup_dns:
>>>> +    if setup_dns:
>>>>         print "BIND DNS server will be configured to serve IPA domain 
>>>> with:"
>>>>         print "Forwarders:    %s" % ("No forwarders" if not dns_forwarders 
>>>> \
>>>>                 else ", ".join([str(ip) for ip in dns_forwarders]))
>>>> @@ -1102,7 +1113,7 @@ def main():
>>>>                persistent_search=options.persistent_search,
>>>>                serial_autoincrement=options.serial_autoincrement,
>>>>                ca_configured=not options.selfsign)
>>>> -    if options.setup_dns:
>>>> +    if setup_dns:
>>>>         api.Backend.ldap2.connect(bind_dn=DN(('cn', 'Directory Manager')),
> bind_pw=dm_password)
>>>>
>>>>         bind.create_instance()
>>>> @@ -1147,11 +1158,11 @@ def main():
>>>>     print "\t\t  * 80, 443: HTTP/HTTPS"
>>>>     print "\t\t  * 389, 636: LDAP/LDAPS"
>>>>     print "\t\t  * 88, 464: kerberos"
>>>> -    if options.setup_dns:
>>>> +    if setup_dns:
>>>>         print "\t\t  * 53: bind"
>>>>     print "\t\tUDP Ports:"
>>>>     print "\t\t  * 88, 464: kerberos"
>>>> -    if options.setup_dns:
>>>> +    if setup_dns:
>>>>         print "\t\t  * 53: bind"
>>>>     if options.conf_ntp:
>>>>         print "\t\t  * 123: ntp"
>>>>
>>>>
>>>>
>>>>
>>>> -------------- next part --------------
>>>> An HTML attachment was scrubbed...
>>>> URL:
> <https://www.redhat.com/archives/freeipa-devel/attachments/20130213/8be3e343/attachment.html>
>>>>
>>>> ------------------------------
>>>>
>>>> _______________________________________________
>>>> Freeipa-devel mailing list
>>>> Freeipa-devel@redhat.com
>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>
>>>> End of Freeipa-devel Digest, Vol 69, Issue 49
>>>> *********************************************
>>>
>>> _______________________________________________
>>> Freeipa-devel mailing list
>>> Freeipa-devel@redhat.com
>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>
>>
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
> 
> 
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
> 

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

Reply via email to