[PATCH conductor] fix conductor spec suite on ruby 1.9

Mo Morsi mmorsi at redhat.com
Wed Feb 29 11:11:37 UTC 2012


On 02/28/2012 10:49 AM, Jason Guiditta wrote:
> On 23/02/12 16:20 -0500, Mo Morsi wrote:
>>  spec suite now fully runs successfully
>>
>
> Are all patches pushed that this depends on?  If not, which are
> needed?  Maybe I can try an get the pre-reqs tested today if they have
> not yet been reviewed.

There are two outstanding patches adding Ruby 1.9 support besides this
one. I had previously sent them out on

https://fedorahosted.org/pipermail/aeolus-devel/2012-February/009116.html
https://fedorahosted.org/pipermail/aeolus-devel/2012-February/009117.html


>
>> ---
>> src/app/controllers/application_controller.rb      |    2 +-
>> src/app/controllers/deployables_controller.rb      |    2 +-
>> .../controllers/permissions_controller_spec.rb     |    2 +-
>> src/spec/controllers/pools_controller_spec.rb      |    5 ++++-
>> 4 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/app/controllers/application_controller.rb
>> b/src/app/controllers/application_controller.rb
>> index be2ded3..aa80e44 100644
>> --- a/src/app/controllers/application_controller.rb
>> +++ b/src/app/controllers/application_controller.rb
>> @@ -112,7 +112,7 @@ class ApplicationController < ActionController::Base
>>   end
>>
>>   # Returns an array of ids from params[:id], params[:ids].
>> -  def ids_list(other_attrs=[])
>> +  def ids_list(*other_attrs)
> Does ruby 1.9 really not support setting a default of an empty array
> in the method definition?  If so, this sounds kind of like a loss of
> functionality in the language.

There are various features of the language that have been added  /
dropped for various reasons w/ Ruby 1.9

That being said, setting default values of method parameters is still
supported. This was the case (among others) where our application was
simply incorrect. During the course of my conversion I noticed some
logic errors that didn't manifest themselves until I ran Aeolus against
Ruby 1.9.

The reason this particular case is incorrect is that we currently don't
verify the type of the 'other_attrs' input parameter to 'ids_list'.
Rather we were setting other_attrs to an empty array with the assumption
that if the default value _isn't_ used, and the user specifying a
specific list of 'other_attrs' to accept, it will come in the form of an
array.

This isn't the case though, look at  pools_controller.rb line 184, where
we pass a string into this method.

This was all fine in ruby 1.8 because the only constraint 'ids_list'
puts on 'other_attrs' is that the 'each' method is a valid method for
the type of that parameter (duck-typing remember).

In ruby 1.8 string defined the 'each' method, so this wasn't a problem
(though probably resulted in unintended behaviour). In ruby 1.9, the
'each' method on string was removed, causing various errors in the
application (this was manifested in a few other places as well,
resulting in changes there).

I changed it to the intended behaviour, where 'other_attrs' will simply
act as a parameter bucket, catching any / all parameters passed into the
method as an array for further processing.



>
>>     other_attrs.each do |attr_key|
>>       return params[attr_key].to_a if params.include?(attr_key)
>>     end
>> diff --git a/src/app/controllers/deployables_controller.rb
>> b/src/app/controllers/deployables_controller.rb
>> index e37bc4a..cd66133 100644
>> --- a/src/app/controllers/deployables_controller.rb
>> +++ b/src/app/controllers/deployables_controller.rb
>> @@ -116,7 +116,7 @@ class DeployablesController < ApplicationController
>>
>>     require_privilege(Privilege::CREATE, Deployable)
>>     @deployable = Deployable.new(params[:deployable])
>> -    @selected_catalogs = Catalog.find(params[:catalog_id].to_a)
>> +    @selected_catalogs = Catalog.find(Array(params[:catalog_id]))
>
> Same comment here - you really have to explicitly pass in 'Array'' to
> the find method?  Is so, this is a bug step back for ruby.

In this case the 'to_a' method was removed from the String class in ruby
1.9. Invoking 'Array' as such is an alternate way to accomplish the same
thing (the ruby 1.9 way)

I'm assuming we wanted to pass an array of ids into the find method
here, eg we're invoking 'to_a' and assigning the result of 'find' to a
variable w/ a name indicating as such (selected_catalogs as opposed to
selected_catalog)

If this is not the case, and we just want to return one selected
catalog, then I agree this should be changed.


Shout out if I can offer more help / elaborations. Most of the language
is the same between Ruby 1.8 / 1.9 but there are a few tricky spots such
as those mentioned above that we will have to look out for.

  -Mo



More information about the aeolus-devel mailing list