[Kimchi-devel] [PATCH] issue #447: Check download URL prior to start Task

[Aline Manera]

On 09/25/2014 11:43 AM, Crístian Viana wrote:
> On 24-09-2014 23:08, Aline Manera wrote:
>> +        # Verify if the URL is valid
>> +        if create_param ==  'url':
>> +            url = params['url']
>> +            try:
>> +                urllib2.urlopen(url)
>
> We should validate the specific parameters for the 'url' case inside 
> the function '_create_volume_with_url', just like we check if the 
> upload file exists before reading it inside the function 
> '_create_volume_with_file'.

I have done it in my first implementation but it does not work well on 
UI perspective.

Let me explain.

If we do this verification on _create_volume_with_url means the POST 
/storagepools/<pool>/storagevolumes will return 202 (success)
On UI side, the "Add volume" dialog will be closed and a new "volume" is 
displayed in the pool indicating it is being downloaded.
But as far as the new volume is added, it appears as "Failed" as the 
Task finished with a failed status as the URL is not valid.

Doing the verification prior to start the Task means POST 
/storagepools/<pool>/storagevolumes will return 500 (failed)
On UI side, the "Add volume" dialog will keep open, so user can modify 
the URL and only see a "in progress volume" when it is really in progress.


>
> Actually, I think the best way to fix this problem is by enlarging the 
> try/except block to include the existing urllib2.urlopen call. We 
> don't need to make two requests. That call will raise the same 
> exception there. The problem now is that it's not included inside the 
> try/except block, hence the "Unexpected exception".
>
> But if we do keep this approach, you still need to close the return 
> variable of "urllib2.urlopen".

Good point. I will do that on V2.