On 18/06/10 17:18 +0200, David Lutterkort wrote: >On Thu, 2010-06-17 at 18:05 +0100, [email protected] wrote: >> From: marios <[email protected]> >> >> --- >> 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.
I'm sorry for this. First time I used this for authentification feature to extend Instance class with feature-specific attributes. I didn't know that this can be a bad approach. I'll post a patch with improved authentification stuff and this issue will be no longer exists. >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 ? Sure, it's better approach that mine. I was wrong because I was worried from NoMethodErrors ;-) -- Michal -- -------------------------------------------------------- Michal Fojtik, [email protected], +420 532 294 4307 Ruby / Ruby On Rails Developer Deltacloud API: http://deltacloud.org -------------------------------------------------------- _______________________________________________ deltacloud-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/deltacloud-devel
