On 03/08/2011 04:29 AM, Uwe Gansert wrote:

Hi,

I've included some comments inline.

diff --git 
a/java/code/src/com/redhat/rhn/domain/kickstart/KickstartInstallType.java 
b/java/code/src/com/redhat/rhn/domain/kickstart/KickstartInstallType.java
index a3f613a..ef92ce6 100644
--- a/java/code/src/com/redhat/rhn/domain/kickstart/KickstartInstallType.java
+++ b/java/code/src/com/redhat/rhn/domain/kickstart/KickstartInstallType.java
@@ -31,6 +31,7 @@ public class KickstartInstallType extends BaseDomainHelper {
      public static final String RHEL_6 = "rhel_6";
      public static final String FEDORA = "fedora";
      public static final String GENERIC = "generic";
+    public static final String SUSE = "suse";

      private Long id;
      private String label;
@@ -94,6 +95,13 @@ public class KickstartInstallType extends BaseDomainHelper {
      }

      /**
+     * @return true if the installer type is suse.
+     */
+    public boolean isSUSE() {
+        return SUSE.equals(getLabel());
+    }
+
+    /**
       * @return Returns the id.
       */
      public Long getId() {
diff --git 
a/java/code/src/com/redhat/rhn/domain/kickstart/KickstartableTree.java 
b/java/code/src/com/redhat/rhn/domain/kickstart/KickstartableTree.java
index 41e3202..510609f 100644
--- a/java/code/src/com/redhat/rhn/domain/kickstart/KickstartableTree.java
+++ b/java/code/src/com/redhat/rhn/domain/kickstart/KickstartableTree.java
@@ -277,6 +277,10 @@ public class KickstartableTree extends BaseDomainHelper {
          else if (arch.equals("channel-ppc")) {
              return StringUtil.addPath(getAbsolutePath(), 
"/ppc/ppc64/vmlinuz");
          }
+        else if ( this.installType.isSUSE() )
+        {
+            return StringUtil.addPath(getAbsolutePath(), "/boot/" + 
this.getChannel().getChannelArch().getName() + "/loader/linux");
+        }
          else {
              return StringUtil.addPath(getAbsolutePath(), 
"/images/pxeboot/vmlinuz");
          }
@@ -296,6 +300,10 @@ public class KickstartableTree extends BaseDomainHelper {
          else if (arch.equals("channel-ppc")) {
              return StringUtil.addPath(getAbsolutePath(), 
"/ppc/ppc64/ramdisk.image.gz");
          }
+        else if ( this.installType.isSUSE() )
+        {
+            return StringUtil.addPath(getAbsolutePath(), "/boot/" + 
this.getChannel().getChannelArch().getName() + "/loader/initrd");
+        }
          else {
              return StringUtil.addPath(getAbsolutePath(), 
"/images/pxeboot/initrd.img");
          }
diff --git 
a/java/code/src/com/redhat/rhn/frontend/action/kickstart/tree/BaseTreeAction.java
 
b/java/code/src/com/redhat/rhn/frontend/action/kickstart/tree/BaseTreeAction.java
index a2cbbdd..8ccb85f 100644
--- 
a/java/code/src/com/redhat/rhn/frontend/action/kickstart/tree/BaseTreeAction.java
+++ 
b/java/code/src/com/redhat/rhn/frontend/action/kickstart/tree/BaseTreeAction.java
@@ -30,6 +30,7 @@ import org.apache.struts.util.LabelValueBean;

  import java.util.Iterator;
  import java.util.List;
+import java.net.*;


We generally don't include entire packages without our code. Please only include the specific classes that you are using.

So more like  import java.net.InetAddress;

If you are using eclipse, it should pretty much do this for you automatically.


  /**
   * TreeCreate class for creating Kickstart Trees
@@ -103,7 +104,20 @@ public abstract class BaseTreeAction extends 
BaseEditAction {
              lookupKickstartInstallTypeByLabel(form.getString(INSTALL_TYPE));
          bte.setInstallType(type);

-        bte.setKernelOptions(form.getString(KERNEL_OPTS));
+        if( type.isSUSE() ) {
+            String kopts = form.getString(POST_KERNEL_OPTS);
+            if( kopts.indexOf("install=") == -1 ) {
               kopts.contains("install=") would be a tad better here :}


+                try {
+                    java.net.InetAddress localMachine = 
java.net.InetAddress.getLocalHost();
+                    kopts = kopts + " install=http://"; + localMachine.getHostName() 
+"/ks/dist/" + form.getString(LABEL);
+                } catch (UnknownHostException e) {
+                    return new ValidatorError("UnknownHostException by 
getLocalHost/getHostName. Set install=.... parameter manually");
+                }
+            }
+            bte.setKernelOptions( kopts );
+        } else {
+            bte.setKernelOptions(form.getString(KERNEL_OPTS));
+        }

While this works, typically we would prefer to use the request to get the hostname. HttpServletRequest.getLocalName() should get you the hostname that the request comes in on. You would have to change the method signature of processCommandSetters to accept the request, but that is a better way to get the hostname IMHO.

This won't work though if you are using proxy, as you would want to point the client to the proxy and not to the spacewalk server. I'm not sure what the solution here is though.... Ideally you would be able to use a cobbler variable such as $http_server, so you could add --install=http://$http_server/ks/dist/LABEL to the kernel options, but cobbler does not seem to render the kernel opts as a template. This would be a good feature to add to cobbler.

Everything else looks pretty good to me.

-Justin Sherrill
_______________________________________________
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Reply via email to