On Thu, 2010-06-17 at 18:05 +0100, mandreou@redhat.com wrote:
From: marios marios@redhat.com
server/lib/deltacloud/drivers/ec2/ec2_driver.rb | 5 +++-- .../drivers/rimuhosting/rimuhosting_driver.rb | 7 ++++++- server/views/instances/show.html.haml | 5 +++++ server/views/instances/show.xml.haml | 3 +++ 4 files changed, 17 insertions(+), 3 deletions(-)
Patch looks good in general. There's one thing that I don't like:
diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb index 076bfc4..b49c5e5 100644 --- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb +++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb @@ -22,7 +22,7 @@ require 'AWS' class Instance attr_accessor :keyname attr_accessor :authn_error
- attr_accessor :launch_time def authn_feature_failed? return true unless authn_error.nil? end
I hadn't noticed that this extension of the Instance class had slipped in. I am very much against doing this, since it will lead to driver-specific models with ugly ripple effects all the way to the views.
The Instance class should be declared in one place, with all its attributes.
Why can't we check with instance.keyname.nil? and instance.launch_time.nil? if these properties were provided ?
David