deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jvlcek <jvl...@redhat.com>
Subject Re: [PATCH core] DigitalOcean: Added support for :keys collection
Date Tue, 26 Feb 2013 18:52:56 GMT
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, mfojtik@redhat.com wrote:
> From: Michal Fojtik <mfojtik@redhat.com>
>
> - Added :authentication_key feature to instances
>
> Signed-off-by: Michal fojtik <mfojtik@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)


Mime
View raw message