This looks fine to me. I only have 1 very small nit and I'll leave it up
to you to
decide to address it or not.
In the code you use multiple terms to refer to authentication keys.
i.e.:
:authentication_key
:ssh_key_ids
:ssh_pub_key
:public_key
:keyname
'ssh_keys'
'ssh_key'
original_key
def keys()
def key()
def convert_key()
I would suggest, "where possible" to use a consistent name to help
the reader identify that what is being referred to are authentication
keys.
e.g.:
:authentication_key -> :auth_key
:ssh_key_ids -> auth_key_ids
:ssh_pub_key -> pub_auth_key
:public_key -> public_auth_key
:keyname -> auth_keyname
'ssh_keys' -> 'auth_keys' <--- Not sure if this one is possible
'ssh_key' -> 'auth_key' <--- Not sure if this one is possible
original_key -> orig_auth_key
def keys() -> def auth_keys()
def key() -> def auth_key()
def convert_key() -> def convert_auth_key()
This is a total nit and I understand it may not be possible in all
cases but this is the kind of consistency I find helpful.
Joe
On 02/25/2013 07:14 AM, mfoj...@redhat.com wrote:
> From: Michal Fojtik <mfoj...@redhat.com>
>
> - Added :authentication_key feature to instances
>
> Signed-off-by: Michal fojtik <mfoj...@redhat.com>
> ---
> .../drivers/digitalocean/digitalocean_driver.rb | 51
> +++++++++++++++++++++-
> server/lib/deltacloud/models/base_model.rb | 2 +
> server/lib/deltacloud/models/key.rb | 2 +-
> server/views/keys/index.html.haml | 1 +
> server/views/keys/show.html.haml | 12 +++--
> server/views/keys/show.xml.haml | 5 ++-
> 6 files changed, 64 insertions(+), 9 deletions(-)
>
> diff --git
> a/server/lib/deltacloud/drivers/digitalocean/digitalocean_driver.rb
> b/server/lib/deltacloud/drivers/digitalocean/digitalocean_driver.rb
> index 2868e93..fbfc987 100644
> --- a/server/lib/deltacloud/drivers/digitalocean/digitalocean_driver.rb
> +++ b/server/lib/deltacloud/drivers/digitalocean/digitalocean_driver.rb
> @@ -20,7 +20,7 @@ module Deltacloud
> module Digitalocean
> class DigitaloceanDriver < Deltacloud::BaseDriver
>
> - feature :instances, :user_name
> + feature :instances, :user_name, :authentication_key
> feature :images, :owner_id
>
> define_instance_states do
> @@ -45,7 +45,7 @@ module Deltacloud
> size = do_client.get("sizes/#{opts[:id]}")["size"]
> results << hardware_profile_from(size)
> else
> - sizes = do_client.get("sizes")["sizes"].each do |s|
> + do_client.get("sizes")["sizes"].each do |s|
> size = do_client.get("sizes/#{s['id']}")["size"]
> results << hardware_profile_from(size)
> end
> @@ -136,6 +136,7 @@ module Deltacloud
> args.merge!(:region_id => opts[:realm_id]) if opts[:realm_id]
> args.merge!(:size_id => opts[:hwp_id]) if opts[:hwp_id]
> args.merge!(:name => opts[:name] || "inst#{Time.now.to_i}")
> + args.merge!(:ssh_key_ids => opts[:keyname]) if opts[:keyname]
> convert_instance(
> credentials.user,
> client.get("droplets/new", args)['droplet']
> @@ -167,6 +168,42 @@ module Deltacloud
> end
> end
>
> + def keys(credentials, opts={})
> + client = new_client(credentials)
> + safely do
> + client.get('ssh_keys')['ssh_keys'].map do |k|
> + convert_key(k)
> + end
> + end
> + end
> +
> + def key(credentials, opts={})
> + client = new_client(credentials)
> + safely do
> + convert_key(client.get("ssh_keys/#{opts[:id]}")["ssh_key"])
> + end
> + end
> +
> + def destroy_key(credentials, opts={})
> + client = new_client(credentials)
> + original_key = key(credentials, opts)
> + safely do
> + client.get("ssh_keys/#{opts[:id]}/destroy")
> + original_key.state = 'deleted'
> + original_key
> + end
> + end
> +
> + def create_key(credentials, opts={})
> + client = new_client(credentials)
> + convert_key(
> + client.get(
> + "ssh_keys/new",
> + :name => opts[:key_name],
> + :ssh_pub_key => opts[:public_key])['ssh_key']
> + )
> + end
> +
> exceptions do
>
> on(/InternalServerError/) do
> @@ -221,6 +258,16 @@ module Deltacloud
> return 'i386' if name.include? 'x32'
> end
>
> + def convert_key(k)
> + Key.new(
> + :id => k['id'],
> + :name => k['name'],
> + :credential_type => :key,
> + :pem_rsa_key => k['ssh_pub_key'],
> + :state => 'available'
> + )
> + end
> +
> def convert_state(status)
> case status
> when 'active' then 'RUNNING'
> diff --git a/server/lib/deltacloud/models/base_model.rb
> b/server/lib/deltacloud/models/base_model.rb
> index 2553042..613bb1f 100644
> --- a/server/lib/deltacloud/models/base_model.rb
> +++ b/server/lib/deltacloud/models/base_model.rb
> @@ -17,6 +17,8 @@
>
> class BaseModel
>
> + attr_accessor :name, :description
> +
> def initialize(init=nil)
> if ( init )
> @id=init[:id]
> diff --git a/server/lib/deltacloud/models/key.rb
> b/server/lib/deltacloud/models/key.rb
> index 64492fb..6600a81 100644
> --- a/server/lib/deltacloud/models/key.rb
> +++ b/server/lib/deltacloud/models/key.rb
> @@ -24,7 +24,7 @@ class Key < BaseModel
> attr_accessor :state
>
> def name
> - @name || @id
> + super || @id
> end
>
> def is_password?
> diff --git a/server/views/keys/index.html.haml
> b/server/views/keys/index.html.haml
> index 248aca7..43efd76 100644
> --- a/server/views/keys/index.html.haml
> +++ b/server/views/keys/index.html.haml
> @@ -8,6 +8,7 @@
> %a{ :href => key_url(key.id), :'data-ajax' => 'false'}
> %img{ :class => 'ui-link-thumb', :src => '/images/key.png'}
> %h3=key.id
> + %p=key.name
> %p
> - if key.credential_type.eql?(:key)
> = key.fingerprint
> diff --git a/server/views/keys/show.html.haml
> b/server/views/keys/show.html.haml
> index 251efa4..2b2a11e 100644
> --- a/server/views/keys/show.html.haml
> +++ b/server/views/keys/show.html.haml
> @@ -6,6 +6,9 @@
> %li{ :'data-role' => 'list-divider'} Identifier
> %li
> %p{ :'data-role' => 'fieldcontain'}=@key.id
> + %li{ :'data-role' => 'list-divider'} Name
> + %li
> + %p{ :'data-role' => 'fieldcontain'}=@key.name
> - if @key.is_password?
> %li{ :'data-role' => 'list-divider'} Username
> %li
> @@ -14,13 +17,14 @@
> %li
> %p{ :'data-role' => 'fieldcontain'}=@key.password
> - else
> - %li{ :'data-role' => 'list-divider'} Fingerprint
> - %li
> - %p{ :'data-role' => 'fieldcontain'}=@key.fingerprint
> + - if @key.fingerprint
> + %li{ :'data-role' => 'list-divider'} Fingerprint
> + %li
> + %p{ :'data-role' => 'fieldcontain'}=@key.fingerprint
> %li{ :'data-role' => 'list-divider'} PEM key
> %li
> %p{ :'data-role' => 'fieldcontain'}
> - %pre=@key.pem_rsa_key
> + %pre{:style=> 'width:600px;overflow:auto;'}=@key.pem_rsa_key
> %li{ :'data-role' => 'list-divider'} Actions
> %li
> %div{ :'data-role' => 'controlgroup', :'data-type' => "horizontal" }
> diff --git a/server/views/keys/show.xml.haml b/server/views/keys/show.xml.haml
> index 2e98d0d..db0786b 100644
> --- a/server/views/keys/show.xml.haml
> +++ b/server/views/keys/show.xml.haml
> @@ -6,8 +6,9 @@
> - if driver.respond_to?(:destroy_key)
> %link{ :rel => "destroy", :method => "delete", :href =>
> destroy_key_url(@key.id)}
> - if @key.is_key?
> - %fingerprint<
> - =@key.fingerprint
> + - if @key.fingerprint
> + %fingerprint<
> + =@key.fingerprint
> - unless @key.pem_rsa_key.nil?
> %pem
> ~render_cdata(@key.pem_rsa_key)
