Jesus M. Rodriguez wrote:
Mike McCune wrote:
I just pushed a big port of our old perl based kickstart file downloader
into java:
http://git.fedorahosted.org/git/spacewalk.git?p=spacewalk.git;a=commit;h=a183f6001d9dcedf1650fcb58098d517be3413b2
If anyone notices anything odd or busted about kickstarts, let me know
Mike
man I miss the inline commenting ability we had with our old svn commit
emails. Guessing what line numbers you are talking about below is kinda
annoying.
Since I'm on a comment on commits rampage, here are some comments :)
FileUtils
-----------
* Would it be better to have a single try/catch around the code
with the 2 exceptions in the catch?
try {
// all the code in here
} catch (FileNotFoundException fnfe) {
} catch (IOException ioe) {
}
sure. will fix.
* you really don't want to do the is.close() after the while,
you want a finally clause where you check it for null and make sure
to close it there. If for some reason you get an IOexception while
reading you will leave the InputStream open.
ok will fix. annoying that i have to try/catch around the close thou
within the finally block.
MD5Sum
-------
* why didn't you use MD5Crypt class in common.util? or add to it instead
of creating MD5Sum
because I didn't write the class, it was copied from another GPL'ed
project. I first thought about putting it into MD5Crypt but that class
is for crypting strings, not getting sums so having a separate class for
methods for getting MD5Sums seemed appropriate.
sure, they both do stuff with MD5 but that is about all they do that are
similar.
DownloadAction
----------------
* We do a split on the StringUtils, do we know that there is at least
2 items in the array to be confident that String treeLabel = split[2]
will not cause a NullPointer or ArrayIndexException?
ported roughly from perl that did the same thing and never had any
issues with it.
This is a pointless URL and should error out:
http://spacewalk.example.com/ks/dist
ideally it would return a 404 vs a 500 error thou. With the older perl
based handler it also returns a 500 error.
What's interesting is that this probably all could've been a servlet
instead of page action, but tihs already existed so I can understand
not changing it.
if you will note it extends Strut's:
org.apache.struts.actions.DownloadAction;
hence, why it is an Action.
thanks for the review!
Mike
--
Mike McCune
mmccune AT redhat.com
Engineering | Portland, OR
RHN Satellite | 650.567.9039x79248
_______________________________________________
Spacewalk-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/spacewalk-devel
