On 18/06/10 17:18 +0200, David Lutterkort wrote:
On Thu, 2010-06-17 at 18:05 +0100, mandreou(a)redhat.com wrote:
> From: marios <marios(a)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.
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, mfojtik(a)redhat.com, +420 532 294 4307
Ruby / Ruby On Rails Developer
Deltacloud API:
http://deltacloud.org
--------------------------------------------------------