On 05/11/2010 05:35 AM, Martyn Taylor wrote:
> Mo,
>
> Thanks for reviewing this so quickly, I've replied to your comments inline,
>
> Cheers
>
> Martyn
> ----- Original Message -----
> From: "Mohammed Morsi"<[email protected]>
> To: [email protected]
> Cc: [email protected]
> Sent: Monday, May 10, 2010 8:45:15 PM GMT +00:00 GMT Britain, Ireland,
> Portugal
> Subject: Re: [deltacloud-devel] [PATCH] Added Validation for Cloud Account
> Provider Credentials
>
> On 05/10/2010 09:56 AM, [email protected] wrote:
>
>> From: martyntaylor<[email protected]>
>>
>> ---
>> src/app/controllers/cloud_accounts_controller.rb | 2 +-
>> src/app/controllers/provider_controller.rb | 11 ++++++++---
>> src/app/models/cloud_account.rb | 16 ++++++++++++++++
>> src/app/views/cloud_accounts/new.html.erb | 1 +
>> src/features/support/env.rb | 6 ++++++
>> .../controllers/cloud_accounts_controller_spec.rb | 7 +++++++
>> src/spec/spec_helper.rb | 7 +++++++
>> 7 files changed, 46 insertions(+), 4 deletions(-)
>>
>>
> Tried it out and it looks good for the most part, though there are some
> caveats below.
>
>
>
>> diff --git a/src/app/controllers/cloud_accounts_controller.rb
>> b/src/app/controllers/cloud_accounts_controller.rb
>> index 05e024e..23b3113 100644
>> --- a/src/app/controllers/cloud_accounts_controller.rb
>> +++ b/src/app/controllers/cloud_accounts_controller.rb
>> @@ -95,4 +95,4 @@ class CloudAccountsController< ApplicationController
>> end
>> redirect_to :controller => 'provider', :action => 'accounts', :id
>> => provider.id
>> end
>> -end
>> +end
>> \ No newline at end of file
>>
>>
> -What is the change here? I'm not even seeing a whitespace difference.
> -Though that being said, I was unable to apply your patch until I ran
> -dos2unix on it. I'm not sure whose end this is a problem on, I remember
> -having this issue in the past, but thought I had resolved it. Could you
> -tell me if either of your ~/.gitconfig or your aggregator checkout
> -.git/config has a CRLF option configured, and what it is set to. If not,
> -we should see if anyone else has a problem applying it to see who has
> -the issue.
>
> OK, I dont actually think there is a change here. I'm not sure if this is a
> result of my improper use of GIT or whether this has occurred due to some
> obscure reason. Anyways, I think it is safe to ignore this.
>
> Myself and Marios have tried applying the patch on a fresh Aggregator and it
> seems to apply OK for us.
>
>
Since there is no actual change can you resend the patch with this
omitted. I ask because when someone is looking at the git log for the
src/app/controllers/cloud_accounts_controller.rb file, they will see
this commit, which should not be there.
>
>> diff --git a/src/app/controllers/provider_controller.rb
>> b/src/app/controllers/provider_controller.rb
>> index 23b5ba0..092054c 100644
>> --- a/src/app/controllers/provider_controller.rb
>> +++ b/src/app/controllers/provider_controller.rb
>> @@ -76,8 +76,13 @@ class ProviderController< ApplicationController
>> require_privilege(Privilege::ACCOUNT_MODIFY)
>> @acct = CloudAccount.find_or_create(params[:cloud_account])
>> @provider = Provider.find(params[:cloud_account][:provider_id])
>> - @provider.cloud_accounts<< @acct
>> - redirect_to :action => 'accounts', :id => @provider.id
>> + if @acct.save
>> + @provider.cloud_accounts<< @acct
>> + redirect_to :action => 'accounts', :id => @provider.id
>> + else
>> + flash[:notice] = @acct.errors.full_messages[0]
>> + redirect_to :action => 'new_account', :id => @provider.id
>> + end
>> end
>>
>> -end
>> +end
>> \ No newline at end of file
>> diff --git a/src/app/models/cloud_account.rb
>> b/src/app/models/cloud_account.rb
>> index 1d6b3e9..f04187d 100644
>> --- a/src/app/models/cloud_account.rb
>> +++ b/src/app/models/cloud_account.rb
>> @@ -67,4 +67,20 @@ class CloudAccount< ActiveRecord::Base
>> def name
>> username
>> end
>> +
>> + protected
>> + def validate
>> + errors.add_to_base("Login Credentials are Invalid for this Provider")
>> unless valid_credentials?
>> + end
>> +
>> + private
>> + def valid_credentials?
>> + begin
>> + deltacloud = DeltaCloud.new(username, password, provider.url)
>> + deltacloud.instances
>> + rescue Exception => e
>> + return false
>> + end
>> + return true
>> + end
>> end
>>
>>
> -Why not just use the 'connect' method defined above in the CloudAccount
> -class, it does the exact same thing. The Provider validate method does
> -it that way.
>
> We can't just use the connect method here because the connect method only
> connects to the Provider, it does not perform any action that requires
> authentication. Since we are checking the validation of login credentials we
> need to test for an AuthenticationException to see if they are valid. To do
> this I've tried listing the instances (since this needs authentication).
> However, there was some discussion on IRC yesterday about possibly adding a
> "test credentials" type method to the API.
>
>
OK that sounds reasonable. Could you please include a TODO or a FIXME
comment right above the call to "instances" saying that it should be
replaced eventually with the test_credentials call.
>> diff --git a/src/app/views/cloud_accounts/new.html.erb
>> b/src/app/views/cloud_accounts/new.html.erb
>> index 40f32cb..8d4fcc8 100644
>> --- a/src/app/views/cloud_accounts/new.html.erb
>> +++ b/src/app/views/cloud_accounts/new.html.erb
>> @@ -4,6 +4,7 @@
>> <h2 class="greeting">New Cloud Account</h2>
>>
>> <div class="dcloud_form">
>> +<%= error_messages_for 'cloud_account' %>
>> <% form_for @cloud_account, :url => { :action => "create" } do |f|
>> %>
>> <%= f.error_messages %>
>> <%= render :partial => "form", :object => f %>
>> diff --git a/src/features/support/env.rb b/src/features/support/env.rb
>> index 2d6e9a1..91327a8 100644
>> --- a/src/features/support/env.rb
>> +++ b/src/features/support/env.rb
>> @@ -30,6 +30,12 @@ Provider.class_eval do
>> end
>> end
>>
>> +CloudAccount.class_eval do
>> + alias_method :original_validate, :validate
>> + def validate
>> + end
>> +end
>> +
>>
>>
> -I'm still pretty new to rspec / cucumber, but is it wise to overload the
> -validate method at this level? eg in other specs / features we might
> -want to test the validate method itself, especially if it gets expanded.
> -What is the benefit of doing this here?
>
> Good point here, I was in some deliberation over this. After giving this
> some thought I decided to go ahead with overriding the method here. I think
> however, that it maybe better to simply override the valid_credentials?
> method.
>
> The reasoning behind doing this, is that each time a CloudAccount is created
> it tries to validate the credentials. For our tests, we don't have a core
> setup to be able to do this, (the same applies for Provider validation). An
> alternative way to do this is to use mock objects and override the method on
> the individual object. This is how Tomas, overcome the problem in some of
> the spec tests. The problem here is that when running a test that uses a
> REST, generally a new object is created from the parameters specified in the
> call. Since this new object is different from the object we stubbed, the
> method is not overidden.
>
> Also if we really did want to test "valid_credentials?" and also the
> "connect" method on the provider, we need some form of integrated testing, so
> we can integrate core and aggregator, which we currently don't have.
>
> I'm open to suggestion on this tho, so if you can think of a better way to do
> this, it would be great :-)
>
OK I see what your doing here and agree this is probably the best
approach for now for getting around the problem (though once again a
comment above the block explaining this is in order). Ultimately this
is because the mock core api driver will thrown an exception when trying
to pass it credentials correct? (else why don't we just use the core api
that needs to be started for the spec suite to run successfully anyways).
-Mo
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
