On Fri, 2011-02-25 at 12:18 +0100, [email protected] wrote:
> From: Michal Fojtik <[email protected]>
Looks good, a few minor things:
git am complains about lotsa trailing whitespace.
> diff --git a/server/lib/deltacloud/base_driver/features.rb
> b/server/lib/deltacloud/base_driver/features.rb
> index a276314..79e859d 100644
> --- a/server/lib/deltacloud/base_driver/features.rb
> +++ b/server/lib/deltacloud/base_driver/features.rb
> @@ -205,7 +205,7 @@ module Deltacloud
> end
>
> declare_feature :instances, :sandboxing do
> - description "Allow lanuching sandbox images"
> + description "Allow launching sandbox images"
That seems to be spurious
> diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> index d657429..e300d56 100644
> --- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> +++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> @@ -155,6 +155,20 @@ module Deltacloud
> end
> end
>
> + def create_image(credentials, opts={})
> + ec2 = new_client(credentials)
> + instance = instance(credentials, :id => opts[:id])
> + safely do
> + raise BackendFeatureUnsupported::new(415, 'InstanceNotEBS',
> 'Instance must be EBS format to create image') unless instance.ebs?
> + if not instance.state == 'RUNNING'
> + raise BackendFeatureUnsupported::new(415,
> 'InstanceNotRunning', 'Instance must be in running state to create image')
>
> + else
> + new_image_id = ec2.create_image(instance.id, opts[:name],
> opts[:description])
> + image(credentials, :id => new_image_id)
> + end
> + end
Should we really replicate this business logic here ? Shouldn't we leave
it up to EC2 to fail the operation and then report that failure back ?
> diff --git a/server/lib/deltacloud/drivers/mock/mock_driver.rb
> b/server/lib/deltacloud/drivers/mock/mock_driver.rb
> index fa18d77..06de4da 100644
> --- a/server/lib/deltacloud/drivers/mock/mock_driver.rb
> +++ b/server/lib/deltacloud/drivers/mock/mock_driver.rb
> @@ -135,6 +135,31 @@ class MockDriver < Deltacloud::BaseDriver
> images.sort_by{|e| [e.owner_id,e.description]}
> end
>
> + def create_image(credentials, opts={})
> + check_credentials(credentials)
> + ids = Dir[ "#{@storage_root}/images/*.yml" ].collect{|e| File.basename(
> e, ".yml" )}
> + count = 0
> + while true
> + next_id = "img#{count}"
> + break if not ids.include?(next_id)
> + count += 1
> + end
> + safely do
> + image = {
> + :name => opts[:name],
> + :owner_id => 'root',
> + :description => opts[:description],
> + :architecture => 'i386',
> + :state => 'UP'
> + }
> + File.open( "#{@storage_root}/images/#{next_id}.yml", 'w' ) do |f|
> + YAML.dump( image, f )
> + end
> + image[:id] = next_id
> + Image.new(image)
> + end
> + end
It would be super-spiffy if the mock driver would report an error if the
instance's create_image flag is false.
> @@ -171,11 +196,9 @@ class MockDriver < Deltacloud::BaseDriver
>
> count = 0
> while true
> - next_id = "inst" + count.to_s
> - if not ids.include?(next_id)
> - break
> - end
> - count = count + 1
> + next_id = "inst#{count}"
> + break if not ids.include?(next_id)
> + count += 1
That seems to be unrelated cleanup.
> diff --git a/server/lib/deltacloud/models/instance.rb
> b/server/lib/deltacloud/models/instance.rb
> index 6345590..b2a1a0d 100644
> --- a/server/lib/deltacloud/models/instance.rb
> +++ b/server/lib/deltacloud/models/instance.rb
> @@ -32,11 +32,20 @@ class Instance < BaseModel
> attr_accessor :authn_error
> attr_accessor :username
> attr_accessor :password
> + attr_accessor :create_image
> +
> + def can_create_image?
> + self.create_image
> + end
>
> def to_s
> name
> end
>
> + def ebs?
> + return true if 'ebs'.eql?(self.root_type)
> + end
It's a little icky that we have a bit of driver-specific stuff in the
instance object. Since EC2Driver already monkey-patches Instance, maybe
we should just stick it there, too (not that I condone wide-spread
monkey patching ;)
> diff --git a/server/views/instances/show.html.haml
> b/server/views/instances/show.html.haml
> index 76d77ba..9fc0916 100644
> --- a/server/views/instances/show.html.haml
> +++ b/server/views/instances/show.html.haml
> @@ -50,3 +50,8 @@
> %dd
> [email protected] do |action|
> =link_to_action action, self.send(:"#{action}_instance_url",
> @instance.id), instance_action_method(action)
> + %dt
> + %dd
> + - if @instance.can_create_image?
> + =link_to_action 'Create Image',
> url_for("/api/images/new?instance_id=#{@instance.id}"), :get
Shouldn't we also expose that in the XML ?
David
