More patches - RE:New tool: /usr/bin/cobbler-setup

[Partha Aji]

Hey Michael,
I basically fixed most of the Fix Me, removed some unused stuff... Fixed
the cobbler.spec to generate the correct rpms.. Updated the templates to
new stuff.  I have questions on 3 fix mes that I have to implement

      # FIXME: missing code to ensure TFTP is enabled
      # FIXME: missing code to configure firewall (???)
      # FIXME: missing code to ensure cobblerd, httpd, xinetd is 
chkconfig on

I was wondering if I could simply use the cobbler check logic found in
cobbler.cobbler.action_check to do the validation for TFTP enabled and
configuring firewall. Secondly are we sure about wanting chkconfig on
for those services right (I thought sysadmins were wary of those things)..
I was also confused on when to do Subprocess.call( shell=True) vs (shell
= False). I currently use Shell = False unless the thing barfed saying
can;t execute the child process in which case I 'd go for shell = True
.. I had to do shell = True for for  '/sbin/service cobblerd restart',
and "/sbin/service httpd restart" ..  I added the logic to check return
codes and cease execution if it failed.. Please review the attached
patches when you have time and apply them accordingly.. I'll meanwhile
work on doing the TFTP enablement checks...

PS:
I was wondering if you were ok with autogenerating the modules.conf &
settings files  when they install the cobbler RPM.
For example we can have a post script that runs
'/usr/bin/cobbler-setup -a /usr/share/cobbler/installer_templates/defaults'
This will make the templates the defacto place to add all the new config
entries... You can add the default values to installer_templates/defaults
That way we don't need keep installer_templates/modules.conf.template in
sync with config/modules.conf... Please let me know what you think of
the idea... Thanks for the reviews in the last commit..

Partha


> From e7a70fa53d3367edea426ea3d21b6097b8697f17 Mon Sep 17 00:00:00 2001
From: Partha Aji <paji(a)redhat.com&gt;
Date: Thu, 18 Sep 2008 19:38:19 -0400
Subject: [PATCH] made some changes relatedto the fix me comments

---
 scripts/cobbler-setup |  179 +++++++++++++++++--------------------------------
 1 files changed, 62 insertions(+), 117 deletions(-)

diff --git a/scripts/cobbler-setup b/scripts/cobbler-setup
index f835f77..5ecdca7 100755
--- a/scripts/cobbler-setup
+++ b/scripts/cobbler-setup
@@ -33,11 +33,9 @@ import exceptions
 
 # FIXME: use subprocess.call() with shell=False instead
 # FIXME: check all return codes
-from os import system
-
 import sys
 import os
-from decimal import Decimal
+
 
 try:
     import readline
@@ -92,39 +90,6 @@ def help_ask(caption, validators=[], transformers =[], default = None,
required=
                         required = required, max_len = max_len)
     return input
 
-# =========================================================
-
-def to_int(input):
-   """
-   A transformer emthod to convert a string input to an integer
-   """
-
-   try:
-       return int(input)
-   except ValueError:
-       raise AnswerException ('Input is not an integer')
-        
-# =========================================================
-        
-def to_float(input):
-    """
-    A transformer emthod to convert a string input to a float.
-    """
-    try:
-        return float(input)
-    except ValueError, e:
-        raise AnswerException ('Input needs to be a number')
-
-# =========================================================
-
-def to_decimal(input):
-    """
-    A transformer emthod to convert a string input to an decimal
-    """
-    try:
-        return Decimal(input)
-    except:
-        raise AnswerException ('Input needs to be a number (can be a decimal)')  
     
 
 # =========================================================
 
@@ -190,22 +155,21 @@ def yes_no_check():
     return enum_check(["y","n"])
 
 # =========================================================
+def yes_no_params(default = 'y'):
+    return  {
+        "default"      : default, 
+        "validators"   : [yes_no_check()], 
+        "transformers" : [yes_no_translator()]
+    }
 
-def range_check(min_val = None, max_val = None):
-    """
-    Ensures that the numeric value o an input falls between a given range.
-    """
-    def check(input, min_val = min_val, max_val = max_val):
-        input = to_decimal(input)
-        if min_val and max_val and not (min_val <= input <= max_val):
-            raise AnswerException('Input needs to be between %d  and %d ' %
(min_val, max_val))
-        elif min_val and input < min_val:
-            raise AnswerException('Input needs to be >= %d' % min_val)
-        elif max_val and input > max_val:
-            raise AnswerException('Input needs to be <= %d' % max_val)
-
-    return check
-
+# =========================================================
+def translation_params(default, translation):
+    return  {
+            "default" : default,
+            "validators" : [enum_check(translation.keys())],
+            "transformers" : [translator(translation)]
+    }    
+    
 # =========================================================
    
 def setup_server(answers):
@@ -216,15 +180,15 @@ def setup_server(answers):
         parameters
     )
 
+
 # =========================================================
 
 def setup_dhcp(answers):
-    parameters = {
-        "default"      : "y", 
-        "validators"   : [yes_no_check()], 
-        "transformers" : [yes_no_translator()]
-    }
-    ask(answers,'enable_dhcp',"Do you want to enable DHCP management [y/n]
?",parameters)
+    
+    ask(answers,
+        'enable_dhcp',
+        "Do you want to enable DHCP management [y/n] ?",
+        yes_no_params())
 
     if answers['enable_dhcp']:
         answers['next_server'] = answers['server']
@@ -235,20 +199,15 @@ def setup_dhcp(answers):
     if answers['enable_dhcp']:
 
         # DHCP management is either ISC or dnsmasq
-    	translators = { 
+    	translation = { 
            "isc"     : "manage_isc", 
            "dnsmasq" : "manage_dnsmasq" 
         }
-        parameters = {
-           "default" : "isc",
-           "validators" : [enum_check(translators.keys())],
-           "transformers" : [translator(translators)]
-        }
         ask(
             answers, 
             'dhcp_module',
             "Which DNS module do you want to use [isc/dnsmasq] ?",
-            parameters
+           translation_params("isc", translation) 
         )
 
     else:
@@ -258,19 +217,12 @@ def setup_dhcp(answers):
 # =========================================================
 				
 def setup_dns(answers):
-
-    parameters = {
-       "default" : "y", 
-       "validators" : [yes_no_check()],
-       "transformers" : [yes_no_translator()]
-    }
-
     # if the user already is using dnsmasq for DHCP, they must use dnsmasq for DNS
     # if they are not, they get the choice of BIND or no module
 
     if answers["enable_dhcp"] and answers["dhcp_module"] ==
"dnsmasq":
        
-       answers["enable_dns"] = True
+       answers["enable_dns"] = 1
        answers["dns_module"] = "manage_dnsmasq"
 
     else:
@@ -278,83 +230,76 @@ def setup_dns(answers):
        ask(answers,
            'enable_dns', 
            "Do you want to enable DNS management with BIND [y/n] ?",   
-           parameters
+           yes_no_params()
        )
  
        if answers["enable_dns"]:
            answers["dns_module"] = "manage_bind"
+
+
     
 # =========================================================
 
 def setup_pxe(answers):
-    parameters = {
-        "default" : "y", 
-        "validators" : [yes_no_check()],
-        "transformers" : [yes_no_translator()]
-    }
-
     ask(
         answers,
         'pxe_once', 
         "Enable PXE boot loop prevention feature [y/n] ?",
-        parameters
+        yes_no_params()
     )
 
 # =========================================================
 
 def setup_mirrors(answers):
-    parameters = {
-        "default" : "n", 
-        "validators" : [yes_no_check()], 
-        "transformers" : [yes_no_translator()]
-    }
     ask(
         answers,
         'yum_post_install_mirror', 
         "Make cobbler managed yum repos available to installed systems via
yum.repos.d [y/n] ?",
-        parameters
+        yes_no_params()
     )
         
 # =========================================================
      
 
 def setup_remote_config(answers):
-
     ask (answers, "enable_remote_access", 
          "Allow cobbler to be managed by the web and other applications [y/n]
?",
-         dict(default="y", validators=[yes_no_check()], 
-         transformers=[yes_no_translator()]))
+         yes_no_params()
+        )
 
     if answers['enable_remote_access']:
-        translation = dict (testing = "authn_testing",
-                            passthru = "authn_passthru",
-                            denyall = "authn_denyall",
-                            ldap = "authn_ldap",
-                            configfile = "authn_configfile",
-                            spacewalk = "authn_spacewalk"
-        )
+        translation = {
+                "testing" : "authn_testing",
+                "passthru" : "authn_passthru",
+                "denyall" : "authn_denyall",
+                "ldap" : "authn_ldap",
+                "configfile" : "authn_configfile",
+                "spacewalk" :"authn_spacewalk"
+        }
+
         
-        ask (answers, "authn_module",
+        ask (answers, 
+             "authn_module",
              "Which authentication module do you want to use [%s] ?" %
"/".join(translation.keys()),
-             dict(default="denyall",
-             validators=[enum_check(translation.keys())],
-             transformers= [translator(translation)]))
+             translation_params("denyall", translation))
 
         if answers['authn_module'] == 'authn_configfile':
 
             print "* Updating cobbler user's password in user.digest file"
             os.system('htdigest -c users.digest Cobbler cobbler')
             print "* users can run 'htdigest /etc/users.digest Cobbler
$username' later to add more users & change cobbler password"
+        
+        translation = {
+                "allowall" : "authz_allowall",
+                "ownership" : "authz_ownership",
+                "configfile" : "authz_configfile"
+        }
 
-        translation = dict (allowall = "authz_allowall",
-                            ownership = "authz_ownership",
-                            configfile = "authz_configfile")
-
-        ask (answers, "authz_module",
-                      "Which authorization module do you want to use [%s] ?" %
"/".join(translation.keys()),
-                      dict(default="allowall",
-                      validators=[enum_check(translation.keys())],
-                      transformers= [translator(translation)]))
+        ask (answers, 
+             "authz_module",
+             "Which authorization module do you want to use [%s] ?" %
"/".join(translation.keys()),
+              translation_params("allowall", translation))
+                      
 
     else: 
 
@@ -417,6 +362,14 @@ def copy_settings():
 # =========================================================
         
 def main():
+    parser = optparse.OptionParser()
+    setup(parser)
+    (options, args) = parser.parse_args()
+    answers = {}
+    # options.file is the name of the answers file..
+    if options.file:
+        data = yaml.loadFile(parser.file).next()
+        answers.update(data)
 
     print ""
     print "**********************************************"
@@ -433,15 +386,7 @@ def main():
     print "**********************************************"
     print ""
 
-    parser = optparse.OptionParser()
-    setup(parser)
-    (options, args) = parser.parse_args()
 
-    answers = {}
-    # FIXME: add --file with help to option parser
-    #if options.file:
-    #    data = yaml.loadFile(parser.file).next()
-    #    answers.update(data)
     
     setup_server(answers)
     setup_dhcp(answers)
-- 
1.5.5.1



> From f944d0596a655dd1213ae12f135d64364a5182b6 Mon Sep 17 00:00:00 2001
From: Partha Aji <paji(a)redhat.com&gt;
Date: Thu, 18 Sep 2008 19:38:35 -0400
Subject: [PATCH] Corrected the cobbler spec..

---
 cobbler.spec |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/cobbler.spec b/cobbler.spec
index e6750ff..ab99ddf 100644
--- a/cobbler.spec
+++ b/cobbler.spec
@@ -122,6 +122,7 @@ test "x$RPM_BUILD_ROOT" != "x" && rm -rf
$RPM_BUILD_ROOT
 %{_bindir}/cobbler
 %{_bindir}/cobbler-ext-nodes
 %{_bindir}/cobblerd
+%{_bindir}/cobbler-setup
 %{_bindir}/cobbler-completion
 
 # %defattr(644,root,root)
@@ -210,6 +211,7 @@ test "x$RPM_BUILD_ROOT" != "x" && rm -rf
$RPM_BUILD_ROOT
 %config(noreplace) /var/lib/cobbler/snippets/pre_partition_select
 %config(noreplace) /var/lib/cobbler/snippets/main_partition_select
 %config(noreplace) /var/lib/cobbler/snippets/post_install_kernel_options
+%config(noreplace) /var/lib/cobbler/snippets/network_config
 /var/lib/cobbler/elilo-3.6-ia64.efi
 /var/lib/cobbler/menu.c32
 %defattr(660,root,root)
-- 
1.5.5.1



> From f135524009cb66737a2b4b8255ea6b6faae1ef2a Mon Sep 17 00:00:00 2001
From: Partha Aji <paji(a)redhat.com&gt;
Date: Fri, 19 Sep 2008 16:03:47 -0400
Subject: [PATCH] Updated the templates .. Also added defualts file, that holds the default
template values ..

---
 cobbler.spec                              |    2 +
 installer_templates/defaults              |   11 ++++
 installer_templates/modules.conf.template |   93 ++++++++++++++++++++++++++--
 installer_templates/settings.template     |   53 ++++++++++++-----
 scripts/cobbler-setup                     |    7 +-
 setup.py                                  |    1 +
 6 files changed, 142 insertions(+), 25 deletions(-)
 create mode 100644 installer_templates/defaults

diff --git a/cobbler.spec b/cobbler.spec
index ab99ddf..4ed1cfd 100644
--- a/cobbler.spec
+++ b/cobbler.spec
@@ -95,6 +95,8 @@ test "x$RPM_BUILD_ROOT" != "x" && rm -rf
$RPM_BUILD_ROOT
 %dir /usr/share/cobbler/installer_templates
 %defattr(744,root,root)
 /usr/share/cobbler/installer_templates/*.template
+%defattr(744,root,root)
+/usr/share/cobbler/installer_templates/defaults
 %defattr(755,apache,apache)
 %dir /usr/share/cobbler/webui_templates
 %defattr(444,apache,apache)
diff --git a/installer_templates/defaults b/installer_templates/defaults
new file mode 100644
index 0000000..34d5a0d
--- /dev/null
+++ b/installer_templates/defaults
@@ -0,0 +1,11 @@
+authn_module:'authn_denyall'
+authz_module:'authz_allowall'
+dns_module:'manage_bind'
+dhcp_module:'manage_isc'
+enable_dhcp:0
+enable_dns:0
+next_server:'127.0.0.1'
+pxe_once:0
+server:'127.0.0.1'
+spacewalk_url:'http://localhost/rpc/api'
+yum_post_install_mirror:0
diff --git a/installer_templates/modules.conf.template
b/installer_templates/modules.conf.template
index 6b66b07..0acd5fd 100644
--- a/installer_templates/modules.conf.template
+++ b/installer_templates/modules.conf.template
@@ -1,26 +1,105 @@
 # this file was auto-generated by /usr/bin/cobbler-setup
 #import time
-$time.asctime()
+#$time.asctime()
 # the previous file is saved as /etc/cobbler/settings.backup
 
-# FIXME: this file is based on an older version of cobbler and needs to be updated to
devel
+# specifies what cobbler modules to load.
 
+# what file/data formats to use for metadata
+#
+# choices:
+#    serializer_catalog (fast, uses .d directories in /var/lib/cobbler/config)
+#    serializer_yaml (original serializer, uses a few text files)
+#
+# for 99% or more of all installations, use serializer_catalog.
+#
+# NOTE:  serializer changes may remove your ability to access old data.
+# serializer_yaml users can change to serializer_catalog w/o manual 
+# migration steps.  Other changes are for new installs only.
 
 [serializers]
-settings = serializer_yaml
-distro = serializer_yaml
-profile = serializer_yaml
-system = serializer_yaml
-repo = serializer_yaml
+settings = serializer_catalog
+distro = serializer_catalog
+profile = serializer_catalog
+system = serializer_catalog
+repo = serializer_catalog
+image = serializer_catalog
+
+# policy: what users can log into the WebUI and Read-Write XMLRPC?
+#
+# choices:
+#    authn_denyall    -- no one (default)
+#    authn_configfile -- use /etc/cobbler/users.digest (for basic setups)
+#    authn_passthru   -- ask Apache to handle it (used for kerberos)
+#    authn_ldap       -- authenticate against LDAP
+#    authn_spacewalk  -- ask Spacewalk/Satellite (experimental)
+#    authn_testing    -- username/password is always testing/testing (debug)
+#    (user supplied)  -- you may write your own module
+#
+# WARNING: this is a security setting, do not choose an option blindly.
+#
+# for more information:
+# https://fedorahosted.org/cobbler/wiki/CobblerWebInterface
+# https://fedorahosted.org/cobbler/wiki/CustomizableSecurity
+# https://fedorahosted.org/cobbler/wiki/CobblerWithKerberos
+# https://fedorahosted.org/cobbler/wiki/CobblerWithLdap
 
 [authentication]
 module = $authn_module
 
+# policy: once a user has been cleared by the WebUI/XMLRPC, what can they do?
+#
+# choices:
+#    authz_allowall   -- full access for all authneticated users (default)
+#    authz_configfile -- determined by /etc/cobbler/users.conf
+#    authz_ownership  -- use users.conf, but add object ownership semantics
+#    (user supplied)  -- you may write your own module
+#
+# WARNING: this is a security setting, do not choose an option blindly.
+#
+# If you want to further restrict cobbler with ACLs for various groups,
+# pick authz_ownership.  authz_allowall does not support ACLs.  configfile
+# does but does not support object ownership which is useful as an additional
+# layer of control.
+
+# for more information:
+# https://fedorahosted.org/cobbler/wiki/CobblerWebInterface
+# https://fedorahosted.org/cobbler/wiki/CustomizableSecurity
+# https://fedorahosted.org/cobbler/wiki/CustomizableAuthorization
+# https://fedorahosted.org/cobbler/wiki/AuthorizationWithOwnership
+# https://fedorahosted.org/cobbler/wiki/AclFeature
+
 [authorization]
 module = $authz_module
 
+# chooses the DNS management engine if manage_dns is enabled
+# in /etc/cobbler/settings, which is off by default.
+#
+# choices:
+#    manage_bind    -- default, uses BIND/named
+#    manage_dnsmasq -- uses dnsmasq, also must select dnsmasq for dhcp below
+#
+# NOTE: more configuration is still required in /etc/cobbler
+#
+# for more information:
+# https://fedorahosted.org/cobbler/wiki/ManageDns
+
 [dns]
 module = $dns_module
 
+# chooses the DHCP management engine if manage_dhcp is enabled
+# in /etc/cobbler/settings, which is off by default.
+#
+# choices:
+#    manage_isc     -- default, uses ISC dhcpd
+#    manage_dnsmasq -- uses dnsmasq, also must select dnsmasq for dns above
+#
+# NOTE: more configuration is still required in /etc/cobbler
+#
+# for more information:
+# https://fedorahosted.org/cobbler/wiki/ManageDhcp
+  
 [dhcp]
 module = $dhcp_module
+
+#--------------------------------------------------
diff --git a/installer_templates/settings.template
b/installer_templates/settings.template
index cbf6e58..96b838c 100644
--- a/installer_templates/settings.template
+++ b/installer_templates/settings.template
@@ -1,16 +1,15 @@
 ---
-# this file was auto-generated by /usr/bin/cobbler-setup
+## this file was auto-generated by /usr/bin/cobbler-setup
 #import time
-$time.asctime()
+#$time.asctime()
 # the previous file is saved as /etc/cobbler/settings.backup
-
-# FIXME: this file is based on an older version of cobbler
-# and needs to be updated to devel/latest
-
 # cobbler settings file
-# run "cobbler sync" after making changes
+# restart cobblerd and consider running "cobbler sync" after making changes
 # (it's a good idea to make backups too)
 #
+# This config file is in YAML 1.0 format
+# see http://yaml.org
+#
 # if 1, cobbler will allow insertions of system records that duplicate
 # the mac address information of other system records.  In general,
 # this is undesirable.
@@ -34,6 +33,7 @@ default_virt_bridge: xenbr0
 # if koan is invoked without --virt-type and no virt-type
 # is set on the profile/system, what virtualization type
 # should be assumed?  Values: xenpv, xenfv, qemu, vmware
+# (NOTE: this does not change what virt_type is chosen by import)
 default_virt_type: xenpv
 
 # use this as the default disk size for virt guests (GB)
@@ -47,6 +47,15 @@ default_virt_ram: 512
 # owner and/or group.  Can be a comma seperated list.
 default_ownership: "admin"
 
+# controls whether cobbler will add each new profile entry to the default
+# PXE boot menu.  This can be over-ridden on a per-profile
+# basis when adding/editing profiles with --enable-menu=0/1.  Users
+# should ordinarily leave this setting enabled unless they are concerned
+# with accidental reinstalls from users who select an entry at the PXE
+# boot menu.  Adding a password to the boot menus templates 
+# may also be a good solution to prevent unwanted reinstallations
+enable_menu: 1
+
 # location for some important binaries and config files 
 # that can vary based on the distribution.
 dhcpd_bin: /usr/sbin/dhcpd
@@ -93,6 +102,18 @@ manage_dns: $enable_dns
 manage_forward_zones: []
 manage_reverse_zones: []
 
+# cobbler has a feature that allows for integration with config management
+# systems such as Puppet.  The following parameters work in conjunction with 
+# --mgmt-classes  and are described in furhter detail at:
+# https://fedorahosted.org/cobbler/wiki/UsingCobblerWithConfigManagementSystem
+mgmt_classes: []
+mgmt_parameters:
+   from_cobbler: 1
+
+# location where cobbler will write its named.conf when BIND dns management is
+# enabled
+named_conf: /etc/named.conf
+
 # if using cobbler with manage_dhcp, put the IP address
 # of the cobbler server here so that PXE booting guests can find it
 # if you do not set this correctly, this will be manifested in TFTP open timeouts.
@@ -156,6 +177,12 @@ server: $server
 # this directory should not be required.
 snippetsdir: /var/lib/cobbler/snippets
 
+# if modules.conf specifies authn_spacewalk, this is the XMLRPC
+# endpoint to authenticate against.  If Satellite/Spacewalk is
+# not in use, ignore this setting entirely.  
+# See fedorahosted.org/spacewalk for details on that project.
+spacewalk_url: $spacewalk_url
+
 # by default, installs are set to send syslog traffic on this port
 # and cobblerd will listen on this port.  syslog data (for installs
 # that support it... RHEL 5 and later, etc) is logged in /var/log/cobbler
@@ -198,12 +225,8 @@ xmlrpc_rw_port: 25152
 # this as 0.  In that case, the cobbler mirrored yum repos are still
 # accessable at http://cobbler.example.org/cblr/repo_mirror and yum
 # configuration can still be done manually.  This is just a shortcut.
-yum_post_install_mirror: $yum_post_install_mirror
-
-# "cobbler repo" support normally uses rsync or reposync.  If --rpm-list
-# is used, it's possible to download only a certain package list, plus
-# dependencies, but --resolve and other flags are not supported in
-# all versions of yumdownloader.  This is a list of what flags
-# to pass to it.  Only change this if you are experiencing problems
-# with "cobbler reposync" and are using --rpm-list.
+yum_post_install_mirror: $yum_post_install_mirror 
+
+# additional flags to yum commands
+yumreposync_flags: "-l"
 yumdownloader_flags: "--resolve"
diff --git a/scripts/cobbler-setup b/scripts/cobbler-setup
index 5ecdca7..198bd66 100755
--- a/scripts/cobbler-setup
+++ b/scripts/cobbler-setup
@@ -235,8 +235,6 @@ def setup_dns(answers):
  
        if answers["enable_dns"]:
            answers["dns_module"] = "manage_bind"
-
-
     
 # =========================================================
 
@@ -396,7 +394,10 @@ def main():
     setup_mirrors(answers)
     
     copy_settings()
-
+    defaults =
yaml.loadFile("/usr/share/cobbler/installer_templates/defaults").next()
+    for key in defaults.keys():
+        if key not in answers: 
+            answers[key] = defaults[key]
     # copy settings /before/ overwriting them
    
templatify("/usr/share/cobbler/installer_templates/settings.template",answers,"/etc/cobbler/settings")
    
templatify("/usr/share/cobbler/installer_templates/modules.conf.template",answers,"/etc/cobbler/modules.conf")
diff --git a/setup.py b/setup.py
index 26d7368..daf3b93 100644
--- a/setup.py
+++ b/setup.py
@@ -126,6 +126,7 @@ if __name__ == "__main__":
                                 # templates for /usr/bin/cobbler-setup
                                 (itemplates,
['installer_templates/modules.conf.template']),
                                 (itemplates,
['installer_templates/settings.template']),
+                                (itemplates, ['installer_templates/defaults']),
 
                                 # kickstart dir
                                 (vl_kick,  []),
-- 
1.5.5.1



> From 24cefd5c902473adb96fac24df0c9b63c273c342 Mon Sep 17 00:00:00 2001
From: Partha Aji <paji(a)redhat.com&gt;
Date: Fri, 19 Sep 2008 16:33:32 -0400
Subject: [PATCH] Corrected a space typo and added defaults to manifest

---
 MANIFEST.in                           |    2 +-
 installer_templates/settings.template |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MANIFEST.in b/MANIFEST.in
index 514775c..4e2e6bc 100644
--- a/MANIFEST.in
+++ b/MANIFEST.in
@@ -17,7 +17,7 @@ include config/completions
 include config/cobbler_bash
 include config/cheetah_macros
 recursive-include templates *.template
-recursive-include installer_templates *.template
+recursive-include installer_templates *.template defaults
 recursive-include kickstarts *.ks
 recursive-include kickstarts *.seed
 include docs/cobbler.1.gz
diff --git a/installer_templates/settings.template
b/installer_templates/settings.template
index 96b838c..ac45af0 100644
--- a/installer_templates/settings.template
+++ b/installer_templates/settings.template
@@ -225,7 +225,7 @@ xmlrpc_rw_port: 25152
 # this as 0.  In that case, the cobbler mirrored yum repos are still
 # accessable at http://cobbler.example.org/cblr/repo_mirror and yum
 # configuration can still be done manually.  This is just a shortcut.
-yum_post_install_mirror: $yum_post_install_mirror 
+yum_post_install_mirror: $yum_post_install_mirror
 
 # additional flags to yum commands
 yumreposync_flags: "-l"
-- 
1.5.5.1



> From f7d0943487f625da469019ec391ae6f15d4b19b3 Mon Sep 17 00:00:00 2001
From: Partha Aji <paji(a)redhat.com&gt;
Date: Fri, 19 Sep 2008 17:53:02 -0400
Subject: [PATCH] Added subprocess.call support

---
 scripts/cobbler-setup |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/scripts/cobbler-setup b/scripts/cobbler-setup
index 198bd66..6be0ba7 100755
--- a/scripts/cobbler-setup
+++ b/scripts/cobbler-setup
@@ -31,11 +31,9 @@ import shutil
 import os.path 
 import exceptions
 
-# FIXME: use subprocess.call() with shell=False instead
-# FIXME: check all return codes
 import sys
 import os
-
+import subprocess
 
 try:
     import readline
@@ -43,6 +41,11 @@ try:
 except: 
     pass
 
+def execute(command, shell = False):
+    if subprocess.call(command, shell = shell) != 0:
+        sys.stderr.write(" -- ERROR:  command '%s' failed executing.
Exiting..\n" % command)
+        sys.exit(1);
+
 class AnswerException(exceptions.Exception):
     """
     Custom exceptions class so we only catch exceptions that we know are input related.
@@ -275,7 +278,6 @@ def setup_remote_config(answers):
                 "spacewalk" :"authn_spacewalk"
         }
 
-        
         ask (answers, 
              "authn_module",
              "Which authentication module do you want to use [%s] ?" %
"/".join(translation.keys()),
@@ -284,7 +286,7 @@ def setup_remote_config(answers):
         if answers['authn_module'] == 'authn_configfile':
 
             print "* Updating cobbler user's password in user.digest file"
-            os.system('htdigest -c users.digest Cobbler cobbler')
+            execute('htdigest -c users.digest Cobbler cobbler',shell=False)
             print "* users can run 'htdigest /etc/users.digest Cobbler
$username' later to add more users & change cobbler password"
         
         translation = {
@@ -330,7 +332,7 @@ def ask(answers, key, question, params=None):
         transformers = params.get("transformers",[]),
         default = params.get("default",None),
         required = params.get("required",True),
-        max_len = params.get("max_len",True)
+        max_len = params.get("max_len",None)
     )
 
     answers[key] = answer
@@ -374,7 +376,7 @@ def main():
     print "Setting up the Cobbler provisioning server."
     print "http://fedorahosted.org/cobbler"
 
-    os.system("rpm -q cobbler")
+    execute("rpm -q cobbler", shell = True)
 
     print ""
     print "if you have already configured cobbler, Ctrl+C now."
@@ -413,8 +415,8 @@ def main():
     print "***********************************************"
     print ""
 
-    os.system("/sbin/service cobblerd restart")
-    os.system("/sbin/service httpd restart")
+    execute("/sbin/service cobblerd restart", shell=True)
+    execute("/sbin/service httpd restart", shell=True)
     
     print ""
     print "***********************************************"
@@ -429,7 +431,7 @@ def main():
     print ""
  
     #print "Running cobbler check ..."
-    os.system("cobbler check")
+    execute("cobbler check", shell=True)
     
     print ""
     print "***********************************************"
-- 
1.5.5.1

[Michael DeHaan]

Partha Aji wrote:
>  Hey Michael,
>  I basically fixed most of the Fix Me, removed some unused stuff... Fixed
>  the cobbler.spec to generate the correct rpms.. Updated the templates to
>  new stuff.  I have questions on 3 fix mes that I have to implement
>
>       # FIXME: missing code to ensure TFTP is enabled
>       # FIXME: missing code to configure firewall (???)
>       # FIXME: missing code to ensure cobblerd, httpd, xinetd is 
>  chkconfig on
>
>  I was wondering if I could simply use the cobbler check logic found in
>  cobbler.cobbler.action_check to do the validation for TFTP enabled and
>  configuring firewall. Secondly are we sure about wanting chkconfig on
>  for those services right (I thought sysadmins were wary of those 
>  things)..
>  I was also confused on when to do Subprocess.call( shell=True) vs (shell
>  = False). I currently use Shell = False unless the thing barfed saying
>  can;t execute the child process in which case I 'd go for shell = True
>  . I had to do shell = True for for  '/sbin/service cobblerd restart',
>  and "/sbin/service httpd restart" ..  I added the logic to check return
>  codes and cease execution if it failed.. Please review the attached
>  patches when you have time and apply them accordingly.. I'll meanwhile
>  work on doing the TFTP enablement checks...
>
>  PS:
>  I was wondering if you were ok with autogenerating the modules.conf &
>  settings files  when they install the cobbler RPM.
>  For example we can have a post script that runs
>  '/usr/bin/cobbler-setup -a 
>  /usr/share/cobbler/installer_templates/defaults'
>  This will make the templates the defacto place to add all the new config
>  entries... You can add the default values to installer_templates/defaults
>  That way we don't need keep installer_templates/modules.conf.template in
>  sync with config/modules.conf... Please let me know what you think of
>  the idea... Thanks for the reviews in the last commit..
>
>  Partha
>  
Partha,

I am a bit confused by the first patch as I've already committed the 
changes to remove the dict() based code and so forth.   As a result, 
these patches do not apply.  I am guessing they were made against the 
copy in the Spacewalk tree, not the copy in the Cobbler tree which 
already had my fixes applied?

--Michael

[Michael DeHaan]

Michael DeHaan wrote:
>  Partha Aji wrote:
> > Hey Michael,
> > I basically fixed most of the Fix Me, removed some unused stuff... Fixed
> > the cobbler.spec to generate the correct rpms.. Updated the templates to
> > new stuff.  I have questions on 3 fix mes that I have to implement
> >
> >      # FIXME: missing code to ensure TFTP is enabled
> >      # FIXME: missing code to configure firewall (???)
> >      # FIXME: missing code to ensure cobblerd, httpd, xinetd is 
> > chkconfig on
> >
> > I was wondering if I could simply use the cobbler check logic found in
> > cobbler.cobbler.action_check to do the validation for TFTP enabled and
> > configuring firewall. Secondly are we sure about wanting chkconfig on
> > for those services right (I thought sysadmins were wary of those 
> > things)..
> > I was also confused on when to do Subprocess.call( shell=True) vs (shell
> > = False). I currently use Shell = False unless the thing barfed saying
> > can;t execute the child process in which case I 'd go for shell = True
> > . I had to do shell = True for for  '/sbin/service cobblerd restart',
> > and "/sbin/service httpd restart" ..  I added the logic to check return
> > codes and cease execution if it failed.. Please review the attached
> > patches when you have time and apply them accordingly.. I'll meanwhile
> > work on doing the TFTP enablement checks...
> >
> > PS:
> > I was wondering if you were ok with autogenerating the modules.conf &
> > settings files  when they install the cobbler RPM.
> > For example we can have a post script that runs
> > '/usr/bin/cobbler-setup -a 
> > /usr/share/cobbler/installer_templates/defaults'
> > This will make the templates the defacto place to add all the new config
> > entries... You can add the default values to 
> > installer_templates/defaults
> > That way we don't need keep installer_templates/modules.conf.template in
> > sync with config/modules.conf... Please let me know what you think of
> > the idea... Thanks for the reviews in the last commit..
> >
> > Partha
> >
>
>  Partha,
>
>  I am a bit confused by the first patch as I've already committed the 
>  changes to remove the dict() based code and so forth.   As a result, 
>  these patches do not apply.  I am guessing they were made against the 
>  copy in the Spacewalk tree, not the copy in the Cobbler tree which 
>  already had my fixes applied?
>
>  --Michael
>
>  
Ok, Partha says I missed one of the dict()'s earlier, so my fault :)

I'll look this over shortly, thanks!

--Michael

[Michael DeHaan]

Michael DeHaan wrote:
>  Partha Aji wrote:
>    
> > Hey Michael,
> > I basically fixed most of the Fix Me, removed some unused stuff... Fixed
> > the cobbler.spec to generate the correct rpms.. Updated the templates to
> > new stuff.  I have questions on 3 fix mes that I have to implement
> >
> >      # FIXME: missing code to ensure TFTP is enabled
> >      # FIXME: missing code to configure firewall (???)
> >      # FIXME: missing code to ensure cobblerd, httpd, xinetd is 
> > chkconfig on
> >
> > I was wondering if I could simply use the cobbler check logic found in
> > cobbler.cobbler.action_check to do the validation for TFTP enabled and
> > configuring firewall. Secondly are we sure about wanting chkconfig on
> > for those services right (I thought sysadmins were wary of those 
> > things)..
> > I was also confused on when to do Subprocess.call( shell=True) vs (shell
> > = False). I currently use Shell = False unless the thing barfed saying
> > can;t execute the child process in which case I 'd go for shell = True
> > . I had to do shell = True for for  '/sbin/service cobblerd restart',
> > and "/sbin/service httpd restart" ..  I added the logic to check return
> > codes and cease execution if it failed.. Please review the attached
> > patches when you have time and apply them accordingly.. I'll meanwhile
> > work on doing the TFTP enablement checks...
> >
> > PS:
> > I was wondering if you were ok with autogenerating the modules.conf &
> > settings files  when they install the cobbler RPM.
> > For example we can have a post script that runs
> > '/usr/bin/cobbler-setup -a 
> > /usr/share/cobbler/installer_templates/defaults'
> > This will make the templates the defacto place to add all the new config
> > entries... You can add the default values to installer_templates/defaults
> > That way we don't need keep installer_templates/modules.conf.template in
> > sync with config/modules.conf... Please let me know what you think of
> > the idea... Thanks for the reviews in the last commit..
> >
> > Partha
> >
> >     
>
>  Partha,
>
>  I am a bit confused by the first patch as I've already committed the 
>  changes to remove the dict() based code and so forth.   As a result, 
>  these patches do not apply.  I am guessing they were made against the 
>  copy in the Spacewalk tree, not the copy in the Cobbler tree which 
>  already had my fixes applied?
>
>  --Michael
>
>
>
>
>
>
>
>  _______________________________________________
>  cobbler mailing list
>  cobbler(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/cobbler
>     
All so far apply accept Patch 2, which was already committed.

Testing now...

[Michael DeHaan]

Michael DeHaan wrote:
>  Michael DeHaan wrote:
>    
> > Partha Aji wrote:
> >   
> >     
> >> Hey Michael,
> >> I basically fixed most of the Fix Me, removed some unused stuff... Fixed
> >> the cobbler.spec to generate the correct rpms.. Updated the templates to
> >> new stuff.  I have questions on 3 fix mes that I have to implement
> >>
> >>      # FIXME: missing code to ensure TFTP is enabled
> >>      # FIXME: missing code to configure firewall (???)
> >>      # FIXME: missing code to ensure cobblerd, httpd, xinetd is 
> >> chkconfig on
> >>
> >> I was wondering if I could simply use the cobbler check logic found in
> >> cobbler.cobbler.action_check to do the validation for TFTP enabled and
> >> configuring firewall. Secondly are we sure about wanting chkconfig on
> >> for those services right (I thought sysadmins were wary of those 
> >> things)..
> >> I was also confused on when to do Subprocess.call( shell=True) vs (shell
> >> = False). I currently use Shell = False unless the thing barfed saying
> >> can;t execute the child process in which case I 'd go for shell = True
> >> . I had to do shell = True for for  '/sbin/service cobblerd restart',
> >> and "/sbin/service httpd restart" ..  I added the logic to check
> return
> >> codes and cease execution if it failed.. Please review the attached
> >> patches when you have time and apply them accordingly.. I'll meanwhile
> >> work on doing the TFTP enablement checks...
> >>
> >> PS:
> >> I was wondering if you were ok with autogenerating the modules.conf &
> >> settings files  when they install the cobbler RPM.
> >> For example we can have a post script that runs
> >> '/usr/bin/cobbler-setup -a 
> >> /usr/share/cobbler/installer_templates/defaults'
> >> This will make the templates the defacto place to add all the new config
> >> entries... You can add the default values to installer_templates/defaults
> >> That way we don't need keep installer_templates/modules.conf.template in
> >> sync with config/modules.conf... Please let me know what you think of
> >> the idea... Thanks for the reviews in the last commit..
> >>
> >> Partha
> >>
> >>     
> >>       
> > Partha,
> >
> > I am a bit confused by the first patch as I've already committed the 
> > changes to remove the dict() based code and so forth.   As a result, 
> > these patches do not apply.  I am guessing they were made against the 
> > copy in the Spacewalk tree, not the copy in the Cobbler tree which 
> > already had my fixes applied?
> >
> > --Michael
> >
> >
> >
> >
> >
> >
> >
> > _______________________________________________
> > cobbler mailing list
> > cobbler(a)lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/cobbler
> >   
> >     
>
>  All so far apply accept Patch 2, which was already committed.
>
>  Testing now...
>
>
>  _______________________________________________
>  cobbler mailing list
>  cobbler(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/cobbler
>     

Ok, so comments...

(A) If using dnsmasq for DHCP, your only choice for DNS management is 
dnsmasq. If using ISC, your only choice is BIND. So this appears wrong 
still:

Which DNS module do you want to use [isc/dnsmasq] ? (default=isc): dnsmasq
Do you want to enable DNS management with BIND [y/n] ? (default=y):

(B) On my system (mdehaan.rdu...) it seems the autodetection code for my 
local system does not work when I press enter for autodetect. If 
autodetect ever returns 127.0.0.1 the installer should require this 
value to be entered manually as a server setting of 127.0.0.1 is not 
going to be usable.

(C) I tweaked cobbler check to not fail as some things cobbler check 
produces are actually warnings. If we want to call this programmatically 
I think what we really want to do is tweak the code behind cobbler check 
to have multiple levels of error codes.

(D) It would probably be a good idea to ask if folks want to primarily 
use Xen or KVM for virtualization. If KVM, the default choices should be 
"virbr0" and "qemu" for bridge & virt type. If xen,
"xenbr0" and "xen". 
This prevents a certain class of questions that frequently comes up on 
IRC :)

(E) If using DHCP management in Cobbler, and the user selects ISC, we 
should also ask if they want to enable OMAPI for dynamic DHCP updates. 
Given that omapi support seems a bit newish (anyone out there fully 
happy with it?) I would recommend the default for this be "N".

(F) We talked about (off-list) making the cobbler build process generate 
config/settings and config/modules.conf using the installer templates 
and the defaults file, so we can keep less copies of these two in 
version control, eliminating the dual maintaince problem. I think this 
is an /outstanding/ idea.

(G) Installer should abort if the effective UID is not 0, else we're 
wasting the users time because we don't have permissions on 
/etc/cobbler/settings and /etc/cobbler/modules.conf

Looks great otherwise, though if you can polish up the above I think it 
would be even better -- then we can add this to all the cobbler setup 
instructions in the manpage and the Wiki, which is definitely a better 
user experience than telling someone to edit the config file and go 
through the guess-and-check process for starters.

Very good, thanks!

--Michael

[Michael DeHaan]

Partha Aji wrote:
>  I have questions on 3 fix mes that I have to implement
>
>       # FIXME: missing code to ensure TFTP is enabled
>       # FIXME: missing code to configure firewall (???)
>       # FIXME: missing code to ensure cobblerd, httpd, xinetd is 
>  chkconfig on
>
>  I was wondering if I could simply use the cobbler check logic found in
>  cobbler.cobbler.action_check to do the validation for TFTP enabled and
>  configuring firewall.  
It checks to see if they are enabled but it does not configure them, so 
this will not be sufficient.

For tftp-server, basically it will need to be a simple sed invocation or 
equivalent.   You should ask before doing this.

I would not auto-configure the firewall as that may not be what the 
admin desires.   I won't like it anyway.




>  Secondly are we sure about wanting chkconfig on
>  for those services right (I thought sysadmins were wary of those 
>  things).. 
A cobbler install with httpd and cobblerd chkconfig off is quite broken, 
especially for Spacewalk purposes.

>  I was also confused on when to do Subprocess.call( shell=True) vs
> (shell
>  = False). I currently use Shell = False unless the thing barfed saying
>  can;t execute the child process in which case I 'd go for shell = True
>  . I had to do shell = True for for  '/sbin/service cobblerd restart',
>  and "/sbin/service httpd restart" .. 
shell=False does not take string arguments, it takes a list, where the 
first argument in the list is the full path to the script/binary.

>  I added the logic to check return
>  codes and cease execution if it failed.. Please review the attached
>  patches when you have time and apply them accordingly.. I'll meanwhile
>  work on doing the TFTP enablement checks...
>
>  PS:
>  I was wondering if you were ok with autogenerating the modules.conf &
>  settings files  when they install the cobbler RPM.
>  For example we can have a post script that runs
>  '/usr/bin/cobbler-setup -a 
>  /usr/share/cobbler/installer_templates/defaults'
>  This will make the templates the defacto place to add all the new config
>  entries...  
This would break .rpmnew, .rpmsave functionality in RPM, so it would not 
be a good option.

>  You can add the default values to installer_templates/defaults
>  That way we don't need keep installer_templates/modules.conf.template in
>  sync with config/modules.conf... Please let me know what you think of
>  the idea... Thanks for the reviews in the last commit..
>
>  Partha
>