deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Fojtik <mfoj...@redhat.com>
Subject Re: [PATCH core 2/2] RHEV-M: Handled exception when backend does not support MachineAdmin. The ignored 'name' parameter for Machine was fixed
Date Wed, 22 Feb 2012 12:16:17 GMT
Hi,

Sure, I'll rebase and split this to smaller commits with proper message.
Thanks for pointing this out :-)

  -- Michal

Michal Fojtik
http://deltacloud.org
mfojtik@redhat.com

On Feb 22, 2012, at 2:03 AM, David Lutterkort wrote:

> On Tue, 2012-02-21 at 15:21 +0100, mfojtik@redhat.com wrote:
>> From: Michal Fojtik <mfojtik@redhat.com>
> 
> Why does the commit message say 'RHEV-M' ? The patch doesn't touch the
> RHEV-M driver. 'CIMI' seems more appropriate.
> 
> Couple more comments:
> 
>> diff --git a/clients/cimi/lib/entities/machine.rb b/clients/cimi/lib/entities/machine.rb
>> index d70d0e9..c17925b 100644
>> --- a/clients/cimi/lib/entities/machine.rb
>> +++ b/clients/cimi/lib/entities/machine.rb 
>> @@ -86,10 +91,10 @@ class CIMI::Frontend::Machine < CIMI::Frontend::Entity
>>       xml.Machine(:xmlns => CIMI::Frontend::CMWG_NAMESPACE) {
>>         xml.name params[:machine][:name]
>>         xml.description params[:machine][:description]
>> -        xml.MachineTemplate {
>> -          xml.MachineConfig( :href => params[:machine][:machine_configuration]
)
>> -          xml.MachineImage( :href => params[:machine][:machine_image] )
>> -          xml.MachineAdmin( :href => params[:machine][:machine_admin] ) unless
params[:machine][:machine_admin].empty?
>> +        xml.machineTemplate {
>> +          xml.machineConfig( :href => params[:machine][:machine_configuration]
)
>> +          xml.machineImage( :href => params[:machine][:machine_image] )
>> +          xml.machineAdmin( :href => params[:machine][:machine_admin] ) unless
params[:machine][:machine_admin].nil?
> 
> This hunk doesn't seem to have anything to do with exception handling;
> should really go into its own patch. (And then I wouldn't have to stare
> at it to figure out it's about changing the capitalization of tags;
> still don't know why the change from empty? to nil?)
> 
>> diff --git a/server/lib/cimi/model/machine.rb b/server/lib/cimi/model/machine.rb
>> index 949a072..88c9f73 100644
>> --- a/server/lib/cimi/model/machine.rb
>> +++ b/server/lib/cimi/model/machine.rb
>> @@ -82,6 +82,7 @@ class CIMI::Model::Machine < CIMI::Model::Base
>>     hardware_profile_id = machine_template['machineConfig'][0]["href"].split('/').last
>>     image_id = machine_template['machineImage'][0]["href"].split('/').last
>>     additional_params = {}
>> +    additional_params[:name] =xml['name'][0] if xml['name']
> 
> This also doesn't seem to be related to MachineAdmin (the reason I harp
> on this is that when I write the NEWS for the next release, changes like
> this just disappear)
> 
>> diff --git a/server/lib/deltacloud/backend_capability.rb b/server/lib/deltacloud/backend_capability.rb
>> index 57e8cd3..05e193c 100644
>> --- a/server/lib/deltacloud/backend_capability.rb
>> +++ b/server/lib/deltacloud/backend_capability.rb
>> @@ -37,7 +37,8 @@ module Deltacloud::BackendCapability
>> 
>>   def check_capability(backend)
>>     if !has_capability?(backend)
>> -      raise Failure.new(capability, "#{capability} capability not supported by backend
#{backend.class.name}")
>> +      e = Failure.new(capability, "#{capability} capability not supported by backend
#{backend.class.name}")
>> +      raise Deltacloud::ExceptionHandler::NotSupported.new(e, nil)
>>     end
>>   end
>> 
>> diff --git a/server/lib/deltacloud/base_driver/exceptions.rb b/server/lib/deltacloud/base_driver/exceptions.rb
>> index e44c89b..3969e7e 100644
>> --- a/server/lib/deltacloud/base_driver/exceptions.rb
>> +++ b/server/lib/deltacloud/base_driver/exceptions.rb
>> @@ -79,6 +79,13 @@ module Deltacloud
>>       end
>>     end
>> 
>> +    class NotSupported < DeltacloudException
>> +      def initialize(e, message)
>> +        message ||= e.message
>> +        super(501, e.class.name, message, e.backtrace)
>> +      end
>> +    end
> 
> Why create the Failure exception, just to take it apart and not use it ?
> 
> David
> 
> 


Mime
View raw message