On Wed, Mar 18, 2009 at 5:24 PM, Mike McCune <mmcc...@redhat.com> wrote:
> 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.
Yeah seriously, it is difficult to comment on large commits.
>
>>
>> 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.
Ok fair enough, makes sense just wanted to make sure that you didn't miss the
existing one.
>
>>
>> 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.
>
jerl? hehe man I haven't used that in a long time. Understood.
> 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.
Ok.
>
>>
>> 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.
yeah I saw that, just seemed like something a servlet would've done, but
Struts Action is fine enough. Thanks for the reply. Good work.
> thanks for the review!
My pleasure
jesus
_______________________________________________
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel
