ACK, with a few nits: Before pushing, remove [revision 2] from the commit message. Also, Gemfile needs a version constraint since we depend on a very new aws gem
On Mon, 2011-05-30 at 17:33 +0300, [email protected] wrote: > From: marios <[email protected]> > diff --git a/server/lib/deltacloud/base_driver/features.rb > b/server/lib/deltacloud/base_driver/features.rb > index 65c4cba..cb25a3b 100644 > --- a/server/lib/deltacloud/base_driver/features.rb > +++ b/server/lib/deltacloud/base_driver/features.rb > @@ -183,11 +183,11 @@ module Deltacloud > end > end > > - declare_feature :instances, :security_group do > - description "Put instance in one or more security groups on launch" > + declare_feature :instances, :firewall do > + description "Put instance in one or more firewalls (security groups) > on launch" > operation :create do > - param :security_group, :array, :optional, nil, > - "Array of security group names" > + param :firewalls, :array, :optional, nil, > + "Array of firewall (security group) id" How is that array encoded ? Should mention that in the description. > diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb > b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb > index 0c0471a..c3911cc 100644 > --- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb > +++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb > @@ -201,7 +203,7 @@ module Deltacloud > instance_options.merge!(:key_name => opts[:keyname]) if > opts[:keyname] > instance_options.merge!(:availability_zone => opts[:realm_id]) if > opts[:realm_id] > instance_options.merge!(:instance_type => opts[:hwp_id]) if > opts[:hwp_id] && opts[:hwp_id].length > 0 > - instance_options.merge!(:group_ids => opts[:security_group]) if > opts[:security_group] > + instance_options.merge!(:group_ids => opts[:firewall]) if > opts[:firewall] The feature declaration calls that param :firewalls, not :firewall > @@ -516,6 +518,7 @@ module Deltacloud > end > end > > + Spurious whitespace change > @@ -764,6 +834,82 @@ module Deltacloud > balancer > end > > + #generate uid from firewall rule parameters (amazon doesn't do this > for us > + def firewall_rule_id(user_id, protocol, from_port, to_port, sources) > + sources_string = "" > + sources.each do |source| > + source.each_pair do |key,value| > + sources_string<< "#{key}=#{value}&" > + end > + sources_string.chomp!("&") > + sources_string<<"|" > + end > + sources_string.chomp!("|") > + > #"type=group&owner=123456789012&name=new_firewall|type=address&family=ipv4&address=192.168.1.1&prefix=24" > + id_string = "user #{user_id}:::protocol #{protocol}:::from_port > #{from_port}:::to_port #{to_port}:::sources #{sources_string}" > + Base64.encode64(id_string) One way to shorten the id is to be less verbose in the fixed parts of id_string. > diff --git a/server/lib/deltacloud/helpers/conversion_helper.rb > b/server/lib/deltacloud/helpers/conversion_helper.rb > index 9a33482..c838b7b 100644 > --- a/server/lib/deltacloud/helpers/conversion_helper.rb > +++ b/server/lib/deltacloud/helpers/conversion_helper.rb > @@ -19,7 +19,8 @@ require 'deltacloud/base_driver' > module ConversionHelper > > def convert_to_json(type, obj) > - if ( [ :image, :realm, :instance, :storage_volume, :storage_snapshot, > :hardware_profile, :key, :bucket, :address ].include?( type ) ) > + if ( [ :image, :realm, :instance, :storage_volume, :storage_snapshot, > :hardware_profile, :key, :bucket, :blob, :firewall, :load_balancer, :address > ].include?( type ) ) > + The addition of :blob and :load_balancer should go into a separate patch. (and there's an extra empty line added) > @@ -27,7 +28,11 @@ module ConversionHelper > type = type.to_s.pluralize > else > data = obj.to_hash > - data.merge!({ :href => self.send(:"#{type}_url", data[:id]) }) > + if type == :blob > + data.merge!({ :href => self.send(:"bucket_url", > "#{data[:bucket]}/#{data[:id]}" ) }) > + else > + data.merge!({ :href => self.send(:"#{type}_url", data[:id]) }) > + end This should also go into a separate 'improve json for blobs' patch. > diff --git a/server/lib/deltacloud/models/bucket.rb > b/server/lib/deltacloud/models/bucket.rb > index 304fc0b..faf0224 100644 > --- a/server/lib/deltacloud/models/bucket.rb > +++ b/server/lib/deltacloud/models/bucket.rb > @@ -24,7 +24,9 @@ class Bucket < BaseModel > > def to_hash > h = self.to_hash_original > - h[:blob_list] = self.blob_list.collect { |blob| { :id => blob, :href => > "/api/buckets/#{self.id}/#{blob.id}"}} > + unless blob_list.nil? > + h[:blob_list] = self.blob_list.collect { |blob| { :id => blob, :href > => "/api/buckets/#{self.id}/#{blob}"}} > + end More for the 'improve json for blobs' patch. > diff --git a/server/public/javascripts/application.js > b/server/public/javascripts/application.js > index 95c9bc2..1c66d78 100644 > --- a/server/public/javascripts/application.js > +++ b/server/public/javascripts/application.js > @@ -51,3 +51,38 @@ function less_fields() > meta_params[0].value = eval(current_val)-1 > } > } > + > +var addresses = 0; > +var groups = 0; > +function make_fields(type) > +{ > + form = document.getElementById("new_rule_form") > + button = document.getElementById("submit_button") Please no tabs, only spaces for indentation. > diff --git a/server/server.rb b/server/server.rb > index 86dd524..1ba166a 100644 > --- a/server/server.rb > +++ b/server/server.rb > @@ -768,7 +768,7 @@ get '/api/buckets/:bucket/:blob' do > respond_to do |format| > format.html { haml :"blobs/show" } > format.xml { haml :"blobs/show" } > - format.json { convert_to_json(blobs, @blob) } > + format.json { convert_to_json(:blob, @blob) } More for the 'improve json for blobs' patch. > @@ -853,6 +853,7 @@ collection :buckets do > > end > > + .. and another hunk that shouldn't be there. David
