Hi Martyn, It's a good work and I like the patch. I have a few suggestion that I think would make the code shorter and clearer. Feel free to ignore them if you disagree (or we can talk about it on IRC).
Anyway, I noticed a small bug that happens when we hook this up to the `mock` driver. All the comments are inline. Thanks, Thomas On 12/02/2010 12:57 PM, [email protected] wrote: > From: Martyn Taylor<[email protected]> > > --- > src/app/models/hardware_profile.rb | 67 ++++++++ > src/spec/factories/hardware_profile.rb | 1 + > src/spec/factories/hardware_profile_property.rb | 30 ++++ > src/spec/factories/property_enum_entry.rb | 4 - > src/spec/models/hardware_profile_spec.rb | 186 > +++++++++++++++++++++++ > 5 files changed, 284 insertions(+), 4 deletions(-) > > diff --git a/src/app/models/hardware_profile.rb > b/src/app/models/hardware_profile.rb > index efd34da..686bce8 100644 > --- a/src/app/models/hardware_profile.rb > +++ b/src/app/models/hardware_profile.rb > @@ -101,4 +101,71 @@ class HardwareProfile< ActiveRecord::Base > end > the_property > end > + > + def self.matching_hwps(hwp) > + matching_hwps = [] > + provider_hwps = HardwareProfile.all(:conditions => 'provider_id IS NOT > NULL') > + > + provider_hwps.each do |phwp| > + if check_properties(hwp, phwp) > + matching_hwps<< phwp > + end > + end > + return matching_hwps I think you can replace the above loop by using Ruby's `select` function: return provider_hwps.select {|phwp| check_properties(hwp, phwp) } > + end > + > + private > + def self.check_properties(hwp1, hwp2) > + check_hwp_property(hwp1.memory, hwp2.memory)&& > + check_hwp_property(hwp1.cpu, hwp2.cpu)&& > + check_hwp_property(hwp1.storage, hwp2.storage)&& > + hwp1.architecture.value == hwp2.architecture.value > + end > + > + def self.check_hwp_property(p1, p2) When I load the hardware providers using the `mock` driver, it crashes. This is because of the `Opaque` HWP provided by the driver. It has all the properties (memory, cpu, storage) set to `nil`. Thus the p2 param is missing the `kind` attribute and that raises an exception. I talked to Michal about it and he said that this is ok with the API -- some providers may not specify each of these values. We should check for the nil here. How to reproduce this: 1. Add a mock provider 2. Add a cloud account 3. run script/console 4. enter the following command to the console: `HardwareProfile.matching_hwps(HardwareProfile.all.first)` > + if p1.kind == 'range' > + calculate_range_match(p1, p2) > + elsif p2.kind == 'range' > + calculate_range_match(p2, p1) > + else > + !(create_array_from_property(p1)& > create_array_from_property(p2)).empty? > + end > + end > + > + def self.calculate_range_match(p1, p2) > + case p2.kind > + when 'range' > + if p1.range_first.to_f> p2.range_last.to_f || p1.range_last.to_f< > p2.range_first.to_f > + return false > + end > + return true I think you could replace this `if` with: return !(p1.range_first.to_f > p2.range_last.to_f || p1.range_last.to_f < p2.range_first.to_f) The expression in the `if` statement already has the true/false value that you want to returning. > + > + when 'enum' > + p2.property_enum_entries.each do |enum| > + if (p1.range_first.to_f..p1.range_last.to_f) === enum.value.to_f > + return true > + end > + end > + return false > + > + when 'fixed' > + return (p1.range_first.to_f..p1.range_last.to_f) === p2.value.to_f > + > + end > + end > + > + def self.create_array_from_property(p) > + case p.kind > + when 'fixed' > + return [p.value.to_f] > + > + when 'enum' > + values = [] > + p.property_enum_entries.each do |enum| > + values<< enum.value.to_f > + end > + return values I think that using `map` would be shorter here: return p.property_enum_entries.map { |enum| enum.value.to_f } > + > + end > + end > end > diff --git a/src/spec/factories/hardware_profile.rb > b/src/spec/factories/hardware_profile.rb > index c4956df..432e679 100644 > --- a/src/spec/factories/hardware_profile.rb > +++ b/src/spec/factories/hardware_profile.rb > @@ -1,5 +1,6 @@ > Factory.define :hardware_profile do |p| > p.sequence(:name) { |n| "hardware_profile#{n}" } > + p.sequence(:external_key) { |n| "hardware_profile_key#{n}" } > end > > Factory.define :mock_hwp1, :parent => :hardware_profile do |p| > diff --git a/src/spec/factories/hardware_profile_property.rb > b/src/spec/factories/hardware_profile_property.rb > index a64ab8a..94453ea 100644 > --- a/src/spec/factories/hardware_profile_property.rb > +++ b/src/spec/factories/hardware_profile_property.rb > @@ -180,3 +180,33 @@ Factory.define :agg_hwp3_arch, :parent => > :hardware_profile_property do |p| > p.unit 'label' > p.value 'i386' > end > + > +Factory.define :hwpp_range, :parent => :hardware_profile_property do |p| > + p.name 'memory' > + p.kind 'range' > + p.unit 'MB' > + p.range_first 256 > + p.range_last 512 > + p.value 256 > +end > + > +Factory.define :hwpp_fixed, :parent => :hardware_profile_property do |p| > + p.name 'memory' > + p.kind 'fixed' > + p.unit 'MB' > + p.value 256 > +end > + > +Factory.define :hwpp_enum, :parent => :hardware_profile_property do |p| > + p.name 'memory' > + p.kind 'enum' > + p.unit 'MB' > + p.value 256 > +end > + > +Factory.define :hwpp_arch, :parent => :hardware_profile_property do |p| > + p.name 'architecture' > + p.kind 'fixed' > + p.unit 'label' > + p.value 'i386' > +end > \ No newline at end of file > diff --git a/src/spec/factories/property_enum_entry.rb > b/src/spec/factories/property_enum_entry.rb > index 78b99d5..6589f45 100644 > --- a/src/spec/factories/property_enum_entry.rb > +++ b/src/spec/factories/property_enum_entry.rb > @@ -3,24 +3,20 @@ end > > Factory.define :mock_hwp2_storage_enum1, :parent => :property_enum_entry > do |e| > e.value 850 > - e.prop_name 'storage' > e.hardware_profile_property { |e| e.association(:mock_hwp2_storage) } > end > > Factory.define :mock_hwp2_storage_enum2, :parent => :property_enum_entry > do |e| > e.value 1024 > - e.prop_name 'storage' > e.hardware_profile_property { |e| e.association(:mock_hwp2_storage) } > end > > Factory.define :agg_hwp2_storage_enum1, :parent => :property_enum_entry do > |e| > e.value 850 > - e.prop_name 'storage' > e.hardware_profile_property { |e| e.association(:agg_hwp2_storage) } > end > > Factory.define :agg_hwp2_storage_enum2, :parent => :property_enum_entry do > |e| > e.value 1024 > - e.prop_name 'storage' > e.hardware_profile_property { |e| e.association(:agg_hwp2_storage) } > end > diff --git a/src/spec/models/hardware_profile_spec.rb > b/src/spec/models/hardware_profile_spec.rb > index bf806e3..bc755f1 100644 > --- a/src/spec/models/hardware_profile_spec.rb > +++ b/src/spec/models/hardware_profile_spec.rb > @@ -85,4 +85,190 @@ describe HardwareProfile do > @hp.memory [email protected]_property(api_prop) > @hp.memory.kind.should equal(@hp.memory.kind.to_s) > end > + > + it "should calculate all the correct matches of provider hardware profiles > against a given hardware profile" do > + provider = Factory(:mock_provider) > + > + # hwpp memory > + hwpp_mem_match_all = Factory(:hwpp_range, :name => 'memory', :unit => > 'MB', :range_first => 1, :range_last => 4096, :value => 256) > + hwpp_mem_match_none = Factory(:hwpp_fixed, :name => 'memory', :unit => > 'MB', :value => 8192) > + hwpp_mem_match_2 = create_hwpp_enum([256, 1024], {:name => 'memory', > :unit => 'MB'}) > + > + hwpp_mem_range = Factory(:hwpp_range, :name => 'memory', :unit => > 'MB', :range_first => 256, :range_last => 512, :value => 256) > + hwpp_mem_fixed = Factory(:hwpp_fixed, :name => 'memory', :unit => > 'MB', :value => 4096) > + hwpp_mem_enum = create_hwpp_enum([1024, 3072, 4096], {:name => > 'memory', :unit => 'MB'}) > + > + # hwpp cpu > + hwpp_cpu_match_all = Factory(:hwpp_range, :name => 'cpu', :unit => > 'count', :range_first => 1, :range_last => 32, :value => 2) > + hwpp_cpu_match_none = Factory(:hwpp_fixed, :name => 'cpu', :unit => > 'count', :value => 64) > + hwpp_cpu_match_2 = create_hwpp_enum([8, 16], {:name => 'cpu', :unit => > 'count'}) > + > + hwpp_cpu_range = Factory(:hwpp_range, :name => 'cpu', :unit => > 'count', :range_first => 1, :range_last => 16, :value => 4) > + hwpp_cpu_fixed = Factory(:hwpp_fixed, :name => 'cpu', :unit => > 'count', :value => 32) > + hwpp_cpu_enum = create_hwpp_enum([16, 32], {:name => 'cpu', :unit => > 'count'}) > + > + # hwpp storage > + hwpp_storage_match_all = Factory(:hwpp_range, :name => 'storage', :unit > => 'GB', :range_first => 100, :range_last => 4000, :value => 250) > + hwpp_storage_match_none = Factory(:hwpp_fixed, :name => 'storage', > :unit => 'GB', :value => 4000) > + hwpp_storage_match_2 = create_hwpp_enum([1000, 2000], {:name => > 'storage', :unit => 'GB'}) > + > + hwpp_storage_range = Factory(:hwpp_range, :name => 'storage', :unit => > 'GB', :range_first => 100, :range_last => 1000, :value => 250) > + hwpp_storage_fixed = Factory(:hwpp_fixed, :name => 'storage', :unit => > 'GB', :value => 3000) > + hwpp_storage_enum = create_hwpp_enum([2000, 4000], {:name => 'storage', > :unit => 'GB'}) > + > + # hwpp arch > + hwpp_arch_i386 = Factory(:hwpp_arch, :value => 'i386') > + > + hwp_match_all = Factory(:hardware_profile, :memory => > hwpp_mem_match_all, > + :cpu => hwpp_cpu_match_all, > + :storage => > hwpp_storage_match_all, > + :architecture => > hwpp_arch_i386) > + > + hwp_match_none = Factory(:hardware_profile, :memory => > hwpp_mem_match_none, > + :cpu => hwpp_cpu_match_none, > + :storage => > hwpp_storage_match_none, > + :architecture => > hwpp_arch_i386) > + > + hwp_match_2 = Factory(:hardware_profile, :memory => hwpp_mem_match_2, > + :cpu => hwpp_cpu_match_2, > + :storage => > hwpp_storage_match_2, > + :architecture => > hwpp_arch_i386) > + > + hwp1 = Factory(:hardware_profile, :memory => hwpp_mem_range, > + :cpu => hwpp_cpu_range, > + :storage => hwpp_storage_range, > + :architecture => hwpp_arch_i386, > + :provider => provider) > + > + hwp2 = Factory(:hardware_profile, :memory => hwpp_mem_fixed, > + :cpu => hwpp_cpu_fixed, > + :storage => hwpp_storage_fixed, > + :architecture => hwpp_arch_i386, > + :provider => provider) > + > + hwp3 = Factory(:hardware_profile, :memory => hwpp_mem_enum, > + :cpu => hwpp_cpu_enum, > + :storage => hwpp_storage_enum, > + :architecture => hwpp_arch_i386, > + :provider => provider) > + > + hwps = [hwp1, hwp2, hwp3] > + (HardwareProfile.matching_hwps(hwp_match_all)& hwps).should == hwps > + (HardwareProfile.matching_hwps(hwp_match_none)& hwps).should == [] > + (HardwareProfile.matching_hwps(hwp_match_2)& hwps).should == [hwp1, > hwp3] > + end > + > + it "should calculate the correct array for hardware profile properties of > kind: 'fixed' and 'enum'" do > + hwp_fixed = Factory(:hwpp_fixed, :value => 256) > + > + enum_array = [256.0, 512.0, 1024.0, 2048.0] > + hwp_enum = create_hwpp_enum(enum_array) > + > + HardwareProfile.create_array_from_property(hwp_fixed).should == [256.0] > + (HardwareProfile.create_array_from_property(hwp_enum)& > enum_array).should == enum_array > + end > + > + it "should determine match for 2 hardware profiles" do > + hwpp_mem_range = Factory(:hwpp_range, :name => 'memory', :unit => > 'MB', :range_first => 256, :range_last => 512, :value => 256) > + hwpp_mem_fixed = Factory(:hwpp_fixed, :name => 'memory', :unit => > 'MB', :value => 1024) > + hwpp_mem_enum = create_hwpp_enum([2048, 3072, 4096], {:name => > 'memory', :unit => 'MB'}) > + > + hwpp_cpu_range = Factory(:hwpp_range, :name => 'cpu', :unit => > 'count', :range_first => 1, :range_last => 4, :value => 2) > + hwpp_cpu_fixed = Factory(:hwpp_fixed, :name => 'cpu', :unit => > 'count', :value => 8) > + hwpp_cpu_enum = create_hwpp_enum([16, 32], {:name => 'cpu', :unit => > 'count'}) > + > + hwpp_storage_range = Factory(:hwpp_range, :name => 'storage', :unit => > 'GB', :range_first => 100, :range_last => 500, :value => 250) > + hwpp_storage_fixed = Factory(:hwpp_fixed, :name => 'storage', :unit => > 'GB', :value => 1000) > + hwpp_storage_enum = create_hwpp_enum([2000, 4000], {:name => 'storage', > :unit => 'GB'}) > + > + hwpp_arch_i386 = Factory(:hwpp_arch, :value => 'i386') > + hwpp_arch_x86_64 = Factory(:hwpp_arch, :value => 'x86_64') > + > + hwp1 = Factory(:hardware_profile, :memory => hwpp_mem_range, :cpu => > hwpp_cpu_range, :storage => hwpp_storage_range, :architecture => > hwpp_arch_i386) > + hwp2 = Factory(:hardware_profile, :memory => hwpp_mem_fixed, :cpu => > hwpp_cpu_fixed, :storage => hwpp_storage_fixed, :architecture => > hwpp_arch_i386) > + hwp3 = Factory(:hardware_profile, :memory => hwpp_mem_enum, :cpu => > hwpp_cpu_enum, :storage => hwpp_storage_enum, :architecture => > hwpp_arch_i386) > + hwp4 = Factory(:hardware_profile, :memory => hwpp_mem_enum, :cpu => > hwpp_cpu_enum, :storage => hwpp_storage_enum, :architecture => > hwpp_arch_x86_64) > + > + HardwareProfile.check_properties(hwp1, hwp1).should == true > + HardwareProfile.check_properties(hwp2, hwp2).should == true > + HardwareProfile.check_properties(hwp3, hwp3).should == true > + > + HardwareProfile.check_properties(hwp1, hwp2).should == false > + HardwareProfile.check_properties(hwp2, hwp3).should == false > + HardwareProfile.check_properties(hwp3, hwp4).should == false > + end > + > + it "should calculate matches for range on hardware profile properties" do > + hwp_range = Factory(:hwpp_range, :range_first => 256, :range_last => > 512, :value => 256) > + > + hwp_range_match = Factory(:hwpp_range, :range_first => 512, :range_last > => 1024, :value => 512) > + hwp_range_fail = Factory(:hwpp_range, :range_first => 2048, :range_last > => 2048, :value => 8192) > + > + hwp_fixed_match = Factory(:hwpp_fixed, :value => 256) > + hwp_fixed_fail = Factory(:hwpp_fixed, :value => 4096) > + > + hwp_enum_match = create_hwpp_enum([256, 512, 1024, 2048]) > + hwp_enum_fail = create_hwpp_enum([2048, 4096, 8192, 16384]) > + > + [hwp_range_match, hwp_fixed_match, hwp_enum_match].each do |hwpp| > + HardwareProfile.calculate_range_match(hwp_range, hwpp).should == true > + end > + > + [hwp_range_fail, hwp_fixed_fail, hwp_enum_fail].each do |hwpp| > + HardwareProfile.calculate_range_match(hwp_range, hwpp).should == false > + end > + end > + > + it "should calculate correct matches for each hwp property" do > + hwp_range1 = Factory(:hwpp_range, :range_first => 256, :range_last => > 512, :value => 512) > + hwp_range2 = Factory(:hwpp_range, :range_first => 512, :range_last => > 1024, :value => 768) > + hwp_range3 = Factory(:hwpp_range, :range_first => 2048, :range_last => > 4096, :value => 3072) > + > + hwp_fixed1 = Factory(:hwpp_fixed, :value => 256) > + hwp_fixed2 = Factory(:hwpp_fixed, :value => 4096) > + hwp_fixed3 = Factory(:hwpp_fixed, :value => 8192) > + > + hwp_enum1 = create_hwpp_enum([256, 512, 1024]) > + hwp_enum2 = create_hwpp_enum([1024, 2048, 3072]) > + hwp_enum3 = create_hwpp_enum([8192, 16384, 32768]) > + > + # Test HWPP Againsts Ranges > + HardwareProfile.check_hwp_property(hwp_range1, hwp_range2).should == true > + HardwareProfile.check_hwp_property(hwp_range1, hwp_range3).should == > false > + > + HardwareProfile.check_hwp_property(hwp_range1, hwp_fixed1).should == true > + HardwareProfile.check_hwp_property(hwp_range1, hwp_fixed3).should == > false > + > + HardwareProfile.check_hwp_property(hwp_range1, hwp_enum1).should == true > + HardwareProfile.check_hwp_property(hwp_range1, hwp_enum3).should == false > + > + # Test HWPP Against Fixed > + HardwareProfile.check_hwp_property(hwp_fixed1, hwp_range1).should == true > + HardwareProfile.check_hwp_property(hwp_fixed1, hwp_range3).should == > false > + > + HardwareProfile.check_hwp_property(hwp_fixed1, hwp_fixed1).should == true > + HardwareProfile.check_hwp_property(hwp_fixed1, hwp_fixed2).should == > false > + > + HardwareProfile.check_hwp_property(hwp_fixed1, hwp_enum1).should == true > + HardwareProfile.check_hwp_property(hwp_fixed1, hwp_enum3).should == false > + > + # Test HWPP Aginsts Enums > + HardwareProfile.check_hwp_property(hwp_enum1, hwp_range1).should == true > + HardwareProfile.check_hwp_property(hwp_enum1, hwp_range3).should == false > + > + HardwareProfile.check_hwp_property(hwp_enum1, hwp_fixed1).should == true > + HardwareProfile.check_hwp_property(hwp_enum1, hwp_fixed2).should == false > + > + HardwareProfile.check_hwp_property(hwp_enum1, hwp_enum2).should == true > + HardwareProfile.check_hwp_property(hwp_enum1, hwp_enum3).should == false > + end > + > + def create_hwpp_enum(value_array, properties = {}) > + hwpp_enum = Factory(:hwpp_enum, properties) > + value_array.each do |value| > + hwpp_enum.property_enum_entries<< Factory(:property_enum_entry, > :value => value, :hardware_profile_property => hwpp_enum) > + end > + return hwpp_enum > + end > + > end _______________________________________________ deltacloud-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/deltacloud-devel
