On 03/22/2013 06:43 AM, Michal Fojtik wrote:
> On 03/21/2013 06:42 PM, Joseph VLcek wrote:
>
>> Michal,
>>
>> This looks great. Many of my comments are really questions regarding
>> why you used Ruby the way you did.
>>
>> I've formated my feedback/questions in the attached patch.
>
> I think you forgot to attach the patch (or my thunderbird is crazy ;)
>
>> I had initially started making code changes but then decided to
>> just leave comments/questions in the code, prefaced wtih "Joe".
>
> Thanks! I'll look at it and will try to respond by mail.
>
>> I've also done some tpyo and grammar fixes and comment changes to
>> attempt to make the comments more consistent.
>>
>> A general question: Are we trying to stick to 80char line lenght?
>
> I'm trying to :-) It is not possible all the time, but where it is
> possible I try to stick to 80 chars. It makes code/patches more readable.
>
> -- Michal
>
>>
>> Hope this helps!
>> Joe V.
>>
>
>
Sorry about the patch not being attached.
Let me know if you don't get it this time.
Joe
>From 08d85f8f130e18659cd65d896dd0e40394e20c90 Mon Sep 17 00:00:00 2001
From: Joe VLcek <jvl...@redhat.com>
Date: Thu, 21 Mar 2013 13:23:50 -0400
Subject: [PATCH] JoeV review feedback for mfojtik's client rewrite
---
client/lib/deltacloud/client.rb | 4 +++-
client/lib/deltacloud/client/base_error.rb | 6 +----
client/lib/deltacloud/client/connection.rb | 28 ++++++++++++----------
client/lib/deltacloud/client/methods.rb | 14 ++++++-----
client/lib/deltacloud/client/methods/address.rb | 7 ++++--
client/lib/deltacloud/client/methods/api.rb | 1 +
.../client/methods/backward_compatiblity.rb | 1 +
client/lib/deltacloud/client/methods/blob.rb | 4 +++-
client/lib/deltacloud/client/methods/bucket.rb | 7 +++---
client/lib/deltacloud/client/methods/driver.rb | 7 +++---
client/lib/deltacloud/client/methods/firewall.rb | 13 ++++++----
.../deltacloud/client/methods/hardware_profile.rb | 7 +++---
client/lib/deltacloud/client/methods/image.rb | 10 ++++----
client/lib/deltacloud/client/methods/instance.rb | 12 +++++-----
.../deltacloud/client/methods/instance_state.rb | 4 ++++
client/lib/deltacloud/client/methods/key.rb | 5 ++--
client/lib/deltacloud/client/methods/realm.rb | 7 +++---
.../deltacloud/client/methods/storage_snapshot.rb | 9 ++++---
.../deltacloud/client/methods/storage_volume.rb | 7 +++---
client/lib/deltacloud/client/models.rb | 12 +++++-----
client/lib/deltacloud/client/models/base.rb | 17 +++++++++----
client/lib/deltacloud/client/models/bucket.rb | 4 ++--
client/lib/deltacloud/client/models/firewall.rb | 5 ++++
.../deltacloud/client/models/hardware_profile.rb | 2 ++
client/lib/deltacloud/client/models/image.rb | 3 +++
client/lib/deltacloud/client/models/instance.rb | 6 +++++
.../lib/deltacloud/client/models/storage_volume.rb | 2 ++
client/lib/deltacloud/core_ext.rb | 2 +-
client/lib/deltacloud/core_ext/fixnum.rb | 2 ++
client/lib/deltacloud/error_response.rb | 13 +++++-----
30 files changed, 131 insertions(+), 90 deletions(-)
diff --git a/client/lib/deltacloud/client.rb b/client/lib/deltacloud/client.rb
index 36200ad..07a4c05 100644
--- a/client/lib/deltacloud/client.rb
+++ b/client/lib/deltacloud/client.rb
@@ -39,7 +39,7 @@ module Deltacloud
require_relative './client/methods/backward_compatiblity'
# Extend Client module with methods that existed in old client
- # and we want to keep them.
+ # that need to be kept.
# Deprecation warnings should be provided to users if they use something
# from these modules.
#
@@ -55,6 +55,8 @@ module Deltacloud
VERSION = '1.1.2'
+ # JoeV should method, valid_connecion? be in
+ # JoeV lib/deltacloud/client/connection.rb ?
# Check if the connection to Deltacloud API is valid
def self.valid_connection?(api_url)
begin
diff --git a/client/lib/deltacloud/client/base_error.rb b/client/lib/deltacloud/client/base_error.rb
index 544aa90..7f8e169 100644
--- a/client/lib/deltacloud/client/base_error.rb
+++ b/client/lib/deltacloud/client/base_error.rb
@@ -24,6 +24,7 @@ module Deltacloud::Client
attr_reader :driver
attr_reader :provider
attr_reader :status
+ attr_reader :original_error
def initialize(opts={})
if opts.is_a? Hash
@@ -38,11 +39,6 @@ module Deltacloud::Client
end
end
- # Return the original XML error message received from Deltacloud API
- def original_error
- @original_error
- end
-
# If the Deltacloud API server error response contain backtrace from
# server,then make this backtrace available as part of this exception
# backtrace
diff --git a/client/lib/deltacloud/client/connection.rb b/client/lib/deltacloud/client/connection.rb
index ad91cb0..69a0a75 100644
--- a/client/lib/deltacloud/client/connection.rb
+++ b/client/lib/deltacloud/client/connection.rb
@@ -23,26 +23,24 @@ module Deltacloud::Client
include Deltacloud::Client::Helpers::Model
- include Deltacloud::Client::Methods::Common
+ include Deltacloud::Client::Methods::Address
include Deltacloud::Client::Methods::Api
include Deltacloud::Client::Methods::BackwardCompatibility
+ include Deltacloud::Client::Methods::Blob
+ include Deltacloud::Client::Methods::Bucket
+ include Deltacloud::Client::Methods::Common
include Deltacloud::Client::Methods::Driver
- include Deltacloud::Client::Methods::Realm
+ include Deltacloud::Client::Methods::Firewall
include Deltacloud::Client::Methods::HardwareProfile
include Deltacloud::Client::Methods::Image
include Deltacloud::Client::Methods::Instance
include Deltacloud::Client::Methods::InstanceState
include Deltacloud::Client::Methods::Key
- include Deltacloud::Client::Methods::StorageVolume
+ include Deltacloud::Client::Methods::Realm
include Deltacloud::Client::Methods::StorageSnapshot
- include Deltacloud::Client::Methods::Address
- include Deltacloud::Client::Methods::Bucket
- include Deltacloud::Client::Methods::Blob
- include Deltacloud::Client::Methods::Firewall
+ include Deltacloud::Client::Methods::StorageVolume
def initialize(opts={})
- @request_driver = opts[:driver]
- @request_provider = opts[:provider]
@connection = Faraday.new(:url => opts[:url]) do |f|
# NOTE: The order of this is somehow important for VCR
# recording.
@@ -53,8 +51,8 @@ module Deltacloud::Client
f.adapter :net_http
end
cache_entrypoint!
- @request_driver ||= current_driver
- @request_provider ||= current_provider
+ @request_driver = opts[:driver] ||= current_driver
+ @request_provider = opts[:provider] ||= current_provider
end
# Change the current driver and return copy of the client
@@ -64,6 +62,7 @@ module Deltacloud::Client
# - api_user -> API user name
# - api_password -> API password
# - api_provider -> API provider (aka API_PROVIDER string)
+ # JoeV - What is "block"?
#
def use(driver_id, api_user, api_password, api_provider=nil, &block)
new_client = self.class.new(
@@ -85,6 +84,7 @@ module Deltacloud::Client
# client.use_provider('eu-west-1') { |p| p.instances }
#
# - provider_id -> API provider (aka API_PROVIDER)
+ # JoeV - What is "block"?
#
def use_provider(provider_id, &block)
new_client = self.clone
@@ -126,8 +126,10 @@ module Deltacloud::Client
def deltacloud_request_headers
headers = {}
headers['Accept'] = 'application/xml'
- headers['X-Deltacloud-Driver'] = @request_driver.to_s if @request_driver
- headers['X-Deltacloud-Provider'] = @request_provider.to_s if @request_provider
+ headers['X-Deltacloud-Driver'] = @request_driver.to_s \
+ if @request_driver
+ headers['X-Deltacloud-Provider'] = @request_provider.to_s \
+ if @request_provider
headers
end
diff --git a/client/lib/deltacloud/client/methods.rb b/client/lib/deltacloud/client/methods.rb
index 0897866..0876806 100644
--- a/client/lib/deltacloud/client/methods.rb
+++ b/client/lib/deltacloud/client/methods.rb
@@ -13,17 +13,19 @@
# License for the specific language governing permissions and limitations
# under the License.
+require_relative './methods/address'
+require_relative './methods/api'
+require_relative './methods/backward_compatibility'
+require_relative './methods/blob'
+require_relative './methods/bucket'
require_relative './methods/common'
require_relative './methods/driver'
-require_relative './methods/realm'
+require_relative './methods/firewall'
require_relative './methods/hardware_profile'
require_relative './methods/image'
require_relative './methods/instance'
require_relative './methods/instance_state'
+require_relative './methods/key'
+require_relative './methods/realm'
require_relative './methods/storage_volume'
require_relative './methods/storage_snapshot'
-require_relative './methods/key'
-require_relative './methods/address'
-require_relative './methods/bucket'
-require_relative './methods/blob'
-require_relative './methods/firewall'
diff --git a/client/lib/deltacloud/client/methods/address.rb b/client/lib/deltacloud/client/methods/address.rb
index 21da5f6..6d365af 100644
--- a/client/lib/deltacloud/client/methods/address.rb
+++ b/client/lib/deltacloud/client/methods/address.rb
@@ -19,8 +19,7 @@ module Deltacloud::Client
# Retrieve list of all address entities
#
- # Filter options:
- #
+ # filter_opts:
# - :id -> Filter entities using 'id' attribute
#
def addresses(filter_opts={})
@@ -53,6 +52,8 @@ module Deltacloud::Client
) do |request|
request.params = { :instance_id => instance_id }
end
+ # JoeV why force status to 202? What if the post fails?
+ # JoeV Is a rescue block needed here to catch post failures?
result.status == 202
end
@@ -60,6 +61,8 @@ module Deltacloud::Client
result = connection.post(
api_uri("/addresses/#{address_id}/disassociate")
)
+ # JoeV why force status to 202? What if the post fails?
+ # JoeV Is a rescue block needed here to catch post failures?
result.status == 202
end
diff --git a/client/lib/deltacloud/client/methods/api.rb b/client/lib/deltacloud/client/methods/api.rb
index 32bd048..350fc44 100644
--- a/client/lib/deltacloud/client/methods/api.rb
+++ b/client/lib/deltacloud/client/methods/api.rb
@@ -40,6 +40,7 @@ module Deltacloud::Client
end
alias_method :api_driver, :current_driver
+ # JoeV - Why not use the api_ prefix for all of the methods defined here?
alias_method :driver_name, :current_driver
# The current provider the @connection is using
diff --git a/client/lib/deltacloud/client/methods/backward_compatiblity.rb b/client/lib/deltacloud/client/methods/backward_compatiblity.rb
index 8718836..9222e03 100644
--- a/client/lib/deltacloud/client/methods/backward_compatiblity.rb
+++ b/client/lib/deltacloud/client/methods/backward_compatiblity.rb
@@ -53,6 +53,7 @@ module Deltacloud::Client
true unless entrypoint.nil?
end
+ # JoeV Why the need for "module ClassMathods"? What does it do for us/
module ClassMethods
def valid_credentials?(api_user, api_password, api_url, opts={})
diff --git a/client/lib/deltacloud/client/methods/blob.rb b/client/lib/deltacloud/client/methods/blob.rb
index 80a3945..266ae2d 100644
--- a/client/lib/deltacloud/client/methods/blob.rb
+++ b/client/lib/deltacloud/client/methods/blob.rb
@@ -17,7 +17,7 @@ module Deltacloud::Client
module Methods
module Blob
- # Retrieve list of all blob entities from given bucket
+ # Retrieve a list of all blob entities from given bucket
#
def blobs(bucket_id=nil)
raise error.new("The :bucket_id cannot be nil.") if bucket_id.nil?
@@ -60,6 +60,8 @@ module Deltacloud::Client
def destroy_blob(bucket_id, blob_id)
must_support! :buckets
r = connection.delete(api_uri("buckets/#{bucket_id}/#{blob_id}"))
+ # JoeV why force status to 204? What if the delete fails?
+ # JoeV Is a rescue block needed here to catch delete failures?
r.status == 204
end
diff --git a/client/lib/deltacloud/client/methods/bucket.rb b/client/lib/deltacloud/client/methods/bucket.rb
index b68cb05..25c0b4f 100644
--- a/client/lib/deltacloud/client/methods/bucket.rb
+++ b/client/lib/deltacloud/client/methods/bucket.rb
@@ -19,9 +19,8 @@ module Deltacloud::Client
# Retrieve list of all bucket entities
#
- # Filter options:
- #
- # - :id -> Filter entities using 'id' attribute
+ # - filter_opts:
+ # - :id -> Filter entities using 'id' attribute
#
def buckets(filter_opts={})
from_collection :buckets,
@@ -39,6 +38,8 @@ module Deltacloud::Client
# Create a new bucket
#
+ # JoeV - Should create_bucket accept create_opts or should this
+ # JoeV comment be removed?
# - create_opts
#
def create_bucket(name)
diff --git a/client/lib/deltacloud/client/methods/driver.rb b/client/lib/deltacloud/client/methods/driver.rb
index 107bcc3..579bcaf0 100644
--- a/client/lib/deltacloud/client/methods/driver.rb
+++ b/client/lib/deltacloud/client/methods/driver.rb
@@ -19,10 +19,9 @@ module Deltacloud::Client
# Retrieve list of all drivers
#
- # Filter options:
- #
- # - :id -> Filter drivers using their 'id'
- # - :state -> Filter drivers by their 'state'
+ # - filter_opt:
+ # - :id -> Filter drivers using their 'id'
+ # - :state -> Filter drivers by their 'state'
#
def drivers(filter_opts={})
from_collection(
diff --git a/client/lib/deltacloud/client/methods/firewall.rb b/client/lib/deltacloud/client/methods/firewall.rb
index 547dfc5..ec1166f 100644
--- a/client/lib/deltacloud/client/methods/firewall.rb
+++ b/client/lib/deltacloud/client/methods/firewall.rb
@@ -19,9 +19,8 @@ module Deltacloud::Client
# Retrieve list of all firewall entities
#
- # Filter options:
- #
- # - :id -> Filter entities using 'id' attribute
+ # - filter_opts:
+ # - :id -> Filter entities using 'id' attribute
#
def firewalls(filter_opts={})
from_collection :firewalls,
@@ -39,8 +38,11 @@ module Deltacloud::Client
# Create a new firewall
#
+ # - name - Name to associate with new firewall
# - create_opts
- #
+ # - JoeV ? create_opts1
+ # - JoeV ? ...
+ # - JoeV ? create_optsn
def create_firewall(name, create_opts={})
create_resource :firewall, { :name => name }.merge(create_opts)
end
@@ -49,6 +51,7 @@ module Deltacloud::Client
destroy_resource :firewall, firewall_id
end
+ # JoeV - User exmples and possible values of arguements might be valuable here.
def add_firewall_rule(firewall_id, protocol, port_from, port_to, opts={})
r = connection.post(api_uri("firewalls/#{firewall_id}/rules")) do |request|
request.params = {
@@ -58,7 +61,7 @@ module Deltacloud::Client
}
# TODO: Add support for sources
end
- model(:firewall).convert(self, r.body)
+ model(:firewall).convert(self, r.body) # JoeV - What is this line for?
end
end
diff --git a/client/lib/deltacloud/client/methods/hardware_profile.rb b/client/lib/deltacloud/client/methods/hardware_profile.rb
index 0cf744c..95c5de4 100644
--- a/client/lib/deltacloud/client/methods/hardware_profile.rb
+++ b/client/lib/deltacloud/client/methods/hardware_profile.rb
@@ -19,9 +19,8 @@ module Deltacloud::Client
# Retrieve list of all hardware_profiles
#
- # Filter options:
- #
- # - :id -> Filter hardware_profiles using their 'id'
+ # - filter_opts:
+ # - :id -> Filter hardware_profiles using their 'id'
#
def hardware_profiles(filter_opts={})
from_collection :hardware_profiles,
@@ -30,7 +29,7 @@ module Deltacloud::Client
# Retrieve the given hardware_profile
#
- # - hardware_profile_id -> hardware_profile to retrieve
+ # - hwp_id -> hardware_profile to retrieve
#
def hardware_profile(hwp_id)
from_resource :hardware_profile,
diff --git a/client/lib/deltacloud/client/methods/image.rb b/client/lib/deltacloud/client/methods/image.rb
index 8e3765b..07ba0f5 100644
--- a/client/lib/deltacloud/client/methods/image.rb
+++ b/client/lib/deltacloud/client/methods/image.rb
@@ -19,11 +19,10 @@ module Deltacloud::Client
# Retrieve list of all images
#
- # Filter options:
- #
- # - :id -> Filter images using their 'id'
- # - :state -> Filter images by their 'state'
- # - :architecture -> Filter images by their 'architecture'
+ # - filter_opts:
+ # - :id -> Filter images using their 'id'
+ # - :state -> Filter images by their 'state'
+ # - :architecture -> Filter images by their 'architecture'
#
def images(filter_opts={})
from_collection :images,
@@ -52,6 +51,7 @@ module Deltacloud::Client
# Destroy given image
# NOTE: This operation might not be supported for all drivers.
+ # JoeV - If possibly not supported is a rescue block needed to handle possible failures?
#
def destroy_image(image_id)
destroy_resource :image, image_id
diff --git a/client/lib/deltacloud/client/methods/instance.rb b/client/lib/deltacloud/client/methods/instance.rb
index 4749d82..506c338 100644
--- a/client/lib/deltacloud/client/methods/instance.rb
+++ b/client/lib/deltacloud/client/methods/instance.rb
@@ -19,11 +19,10 @@ module Deltacloud::Client
# Retrieve list of all instances
#
- # Filter options:
- #
- # - :id -> Filter instances using their 'id'
- # - :state -> Filter instances by their 'state'
- # - :realm_id -> Filter instances based on their 'realm_id'
+ # - filter_opts:
+ # - :id -> Filter instances using their 'id'
+ # - :state -> Filter instances by their 'state'
+ # - :realm_id -> Filter instances based on their 'realm_id'
#
def instances(filter_opts={})
from_collection(
@@ -45,7 +44,8 @@ module Deltacloud::Client
# Create a new instance
#
- # - image_id -> Image to use for instance creation (img1, ami-12345, etc...)
+ # - image_id -> Image to use for instance creation
+ # (img1, ami-12345, etc...)
# - create_opts -> Various options that DC support for the current
# provider.
#
diff --git a/client/lib/deltacloud/client/methods/instance_state.rb b/client/lib/deltacloud/client/methods/instance_state.rb
index 3c853d0..4ac5523 100644
--- a/client/lib/deltacloud/client/methods/instance_state.rb
+++ b/client/lib/deltacloud/client/methods/instance_state.rb
@@ -32,6 +32,10 @@ module Deltacloud::Client
end
end
+ # JoeV Does this require gathering state infor for all instances?
+ # JoeV If so it could be a performance issue when there are a lot
+ # JoeV and it might result in failures if instances are being
+ # JoeV destroyed while the query is happening>
def instance_state(name)
instance_states.find { |s| s.name.to_s.eql?(name.to_s) }
end
diff --git a/client/lib/deltacloud/client/methods/key.rb b/client/lib/deltacloud/client/methods/key.rb
index 8984f23..3e7f48b 100644
--- a/client/lib/deltacloud/client/methods/key.rb
+++ b/client/lib/deltacloud/client/methods/key.rb
@@ -19,9 +19,8 @@ module Deltacloud::Client
# Retrieve list of all key entities
#
- # Filter options:
- #
- # - :id -> Filter entities using 'id' attribute
+ # - filter_opts:
+ # - :id -> Filter entities using 'id' attribute
#
def keys(filter_opts={})
from_collection :keys,
diff --git a/client/lib/deltacloud/client/methods/realm.rb b/client/lib/deltacloud/client/methods/realm.rb
index 41807f0..50d382d 100644
--- a/client/lib/deltacloud/client/methods/realm.rb
+++ b/client/lib/deltacloud/client/methods/realm.rb
@@ -19,10 +19,9 @@ module Deltacloud::Client
# Retrieve list of all realms
#
- # Filter options:
- #
- # - :id -> Filter realms using their 'id'
- # - :state -> Filter realms by their 'state'
+ # - filter_opts:
+ # - :id -> Filter realms using their 'id'
+ # - :state -> Filter realms by their 'state'
#
def realms(filter_opts={})
from_collection :realms,
diff --git a/client/lib/deltacloud/client/methods/storage_snapshot.rb b/client/lib/deltacloud/client/methods/storage_snapshot.rb
index 33c1696..e8c5a6d 100644
--- a/client/lib/deltacloud/client/methods/storage_snapshot.rb
+++ b/client/lib/deltacloud/client/methods/storage_snapshot.rb
@@ -19,9 +19,8 @@ module Deltacloud::Client
# Retrieve list of all storage_snapshot entities
#
- # Filter options:
- #
- # - :id -> Filter entities using 'id' attribute
+ # - filter_options:
+ # - :id -> Filter entities using 'id' attribute
#
def storage_snapshots(filter_opts={})
from_collection :storage_snapshots,
@@ -41,8 +40,8 @@ module Deltacloud::Client
#
# - volume_id -> ID of the +StorageVolume+ to create snapshot from
# - create_opts ->
- # :name -> Name of the StorageSnapshot
- # :description -> Description of the StorageSnapshot
+ # - :name -> Name of the StorageSnapshot
+ # - :description -> Description of the StorageSnapshot
#
def create_storage_snapshot(volume_id, create_opts={})
create_resource :storage_snapshot, create_opts.merge(:volume_id => volume_id)
diff --git a/client/lib/deltacloud/client/methods/storage_volume.rb b/client/lib/deltacloud/client/methods/storage_volume.rb
index be6c4ca..994ffba 100644
--- a/client/lib/deltacloud/client/methods/storage_volume.rb
+++ b/client/lib/deltacloud/client/methods/storage_volume.rb
@@ -19,10 +19,9 @@ module Deltacloud::Client
# Retrieve list of all storage_volumes
#
- # Filter options:
- #
- # - :id -> Filter storage_volumes using their 'id'
- # - :state -> Filter storage_volumes by their 'state'
+ # - filter_opts:
+ # - :id -> Filter storage_volumes using their 'id'
+ # - :state -> Filter storage_volumes by their 'state'
#
def storage_volumes(filter_opts={})
from_collection :storage_volumes,
diff --git a/client/lib/deltacloud/client/models.rb b/client/lib/deltacloud/client/models.rb
index 63b5a24..a6ed222 100644
--- a/client/lib/deltacloud/client/models.rb
+++ b/client/lib/deltacloud/client/models.rb
@@ -13,18 +13,18 @@
# License for the specific language governing permissions and limitations
# under the License.
+require_relative './models/address'
require_relative './models/base'
+require_relative './models/blob'
+require_relative './models/bucket'
require_relative './models/driver'
-require_relative './models/realm'
+require_relative './models/firewall'
require_relative './models/hardware_profile'
require_relative './models/image'
require_relative './models/instance_address'
require_relative './models/instance'
require_relative './models/instance_state'
+require_relative './models/key'
+require_relative './models/realm'
require_relative './models/storage_volume'
require_relative './models/storage_snapshot'
-require_relative './models/key'
-require_relative './models/address'
-require_relative './models/bucket'
-require_relative './models/blob'
-require_relative './models/firewall'
diff --git a/client/lib/deltacloud/client/models/base.rb b/client/lib/deltacloud/client/models/base.rb
index 9f51fb0..c35b89e 100644
--- a/client/lib/deltacloud/client/models/base.rb
+++ b/client/lib/deltacloud/client/models/base.rb
@@ -31,7 +31,7 @@ module Deltacloud::Client
attr_reader :description
# The Base class that other models should inherit from
- # To initialize, you need to suply these mandatory params:
+ # To initialize, you need to supply these mandatory params:
#
# - :_client -> Reference to Client instance
# - :_id -> The 'id' of resource. The '_' is there to avoid conflicts
@@ -46,10 +46,11 @@ module Deltacloud::Client
update_instance_variables!(@options)
end
+ # JoeV Why alias :_id, why not just use :obj_id ?
alias_method :_id, :obj_id
# Populate instance variables in model
- # This method could be also used to update the variables for already
+ # This method could also be used to update the variables for already
# initialized models. Look at +Instance#reload!+ method.
#
def update_instance_variables!(opts={})
@@ -67,6 +68,7 @@ module Deltacloud::Client
# An internal reference to the current Deltacloud::Client::Connection
# instance. Used for implementing the model methods
#
+ # JoeV Why couldn't this method be defined with "attr_reader :client"?
def client
@client
end
@@ -88,18 +90,25 @@ module Deltacloud::Client
# Return the original XML body model was constructed from
# This might help debugging broken XML
#
+ # JoeV Why couldn't this method be defined with "attr_reader :original_body"?
def original_body
@original_body
end
- # The model#id is the old way how to get the Deltacloud API resource
+ # The model#id is the old way for getting the Deltacloud API resource
# 'id'. However this collide with the Ruby Object#id.
#
def id
- warn '[DEPRECATION] `id` is deprecated because of possible conflict with Object#id. Use `_id` instead.'
+ warn '[DEPRECATION] `id` is deprecated because of a possible conflict with Object#id. Use `_id` instead.'
_id
end
+ # JoeV My preference is to not use class << self but instead us the self.<method name>
+ # syntax because when reading this code it's easy to miss the
+ # class << self and not realize that "def from_collection" is actually
+ # "def self.from_collection"
+ # JoeV Or am I mssing something here?
+ #
class << self
# Parse the XML response body from Deltacloud API
diff --git a/client/lib/deltacloud/client/models/bucket.rb b/client/lib/deltacloud/client/models/bucket.rb
index 9a8a856..3a689bf 100644
--- a/client/lib/deltacloud/client/models/bucket.rb
+++ b/client/lib/deltacloud/client/models/bucket.rb
@@ -38,14 +38,14 @@ module Deltacloud::Client
end
# Add a new blob to the bucket.
- # See: +create_blob+
+ # See: methods/blob.rb +create_blob+
#
def add_blob(blob_name, blob_data, blob_create_opts={})
create_blob(_id, blob_name, blob_data, create_opts)
end
# Remove a blob from the bucket
- # See: +destroy_blob+
+ # See: methods/blob.rb +destroy_blob+
#
def remove_blob(blob_id)
destroy_blob(_id, blob_id)
diff --git a/client/lib/deltacloud/client/models/firewall.rb b/client/lib/deltacloud/client/models/firewall.rb
index 9bbe0f2..bc4d58c 100644
--- a/client/lib/deltacloud/client/models/firewall.rb
+++ b/client/lib/deltacloud/client/models/firewall.rb
@@ -26,6 +26,8 @@ module Deltacloud::Client
attr_reader :owner_id
attr_reader :rules
+ # JoeV - What is this? I don't see firewall_reboot() defined anyplace.
+ # JoeV - Is this for future?
# Firewall model methods
#
# def reboot!
@@ -45,6 +47,9 @@ module Deltacloud::Client
}
end
+ # JoeV - what does this class definition withint the Firewall class do?
+ # They both provide a "parse" method.
+
class Rule < Deltacloud::Client::Base
attr_reader :allow_protocol
diff --git a/client/lib/deltacloud/client/models/hardware_profile.rb b/client/lib/deltacloud/client/models/hardware_profile.rb
index 83432b8..c751760 100644
--- a/client/lib/deltacloud/client/models/hardware_profile.rb
+++ b/client/lib/deltacloud/client/models/hardware_profile.rb
@@ -28,6 +28,7 @@ module Deltacloud::Client
}
end
+ # JoeV - Why can't these be defined with attr_reader?
def cpu
property :cpu
end
@@ -54,6 +55,7 @@ module Deltacloud::Client
@properties.find { |p| p.name == name.to_s }
end
+ # JoeV property_klass or should this be property_class?
def self.property_klass(kind)
case kind
when 'enum' then Property::Enum
diff --git a/client/lib/deltacloud/client/models/image.rb b/client/lib/deltacloud/client/models/image.rb
index 8599f41..2158517 100644
--- a/client/lib/deltacloud/client/models/image.rb
+++ b/client/lib/deltacloud/client/models/image.rb
@@ -27,6 +27,9 @@ module Deltacloud::Client
hardware_profile_ids.include? hardware_profile_id
end
+ # JoeV - Why is launch defined here in "models"/image?
+ # JoeV - Shouldn't it be in methods/instance.rb or methods/image.rb ?
+
# Launch the image using +Instance+#+create_instance+ method.
# This method is more strict in checking +HardwareProfile+
# and in case you use incompatible HWP it raise an error.
diff --git a/client/lib/deltacloud/client/models/instance.rb b/client/lib/deltacloud/client/models/instance.rb
index 01e1882..eb984f8 100644
--- a/client/lib/deltacloud/client/models/instance.rb
+++ b/client/lib/deltacloud/client/models/instance.rb
@@ -31,6 +31,9 @@ module Deltacloud::Client
attr_accessor :public_addresses
attr_accessor :private_addresses
+ # JoeV - I thought the models contained the xml parsing.
+ # It's not clear to my why these methods are defined here.
+ # Shouldn't they be defined in methods/instance.rb?
# Destroy the current Instance
#
def destroy!
@@ -86,6 +89,9 @@ module Deltacloud::Client
super
end
+ # JoeV why not use self.parse()? If a reader missed the class << self line
+ # they might not realize parse is a singleton.
+ #
class << self
def parse(xml_body)
diff --git a/client/lib/deltacloud/client/models/storage_volume.rb b/client/lib/deltacloud/client/models/storage_volume.rb
index 26e1e80..91fc290 100644
--- a/client/lib/deltacloud/client/models/storage_volume.rb
+++ b/client/lib/deltacloud/client/models/storage_volume.rb
@@ -30,6 +30,8 @@ module Deltacloud::Client
attr_accessor :kind
attr_accessor :mount
+ # JoeV - Please explain why these methods are here in models
+ # Shouldn't they be in methods/storage_volume.rb
# Check if the current volume is attached to an Instance
#
def attached?
diff --git a/client/lib/deltacloud/core_ext.rb b/client/lib/deltacloud/core_ext.rb
index e85cdba..e8c5247 100644
--- a/client/lib/deltacloud/core_ext.rb
+++ b/client/lib/deltacloud/core_ext.rb
@@ -14,6 +14,6 @@
# under the License.
require_relative './core_ext/element'
-require_relative './core_ext/string'
require_relative './core_ext/fixnum'
+require_relative './core_ext/nil'
require_relative './core_ext/string'
diff --git a/client/lib/deltacloud/core_ext/fixnum.rb b/client/lib/deltacloud/core_ext/fixnum.rb
index 5eb6e1b..767643c 100644
--- a/client/lib/deltacloud/core_ext/fixnum.rb
+++ b/client/lib/deltacloud/core_ext/fixnum.rb
@@ -13,6 +13,8 @@
# License for the specific language governing permissions and limitations
# under the License.
+# JoeV I can't see where this is being used.
+
class Fixnum
def is_redirect?
diff --git a/client/lib/deltacloud/error_response.rb b/client/lib/deltacloud/error_response.rb
index 428b5ec..af663cf 100644
--- a/client/lib/deltacloud/error_response.rb
+++ b/client/lib/deltacloud/error_response.rb
@@ -22,19 +22,19 @@ module Deltacloud
# In case there is no error returned in body, it will try to use
# the generic error reporting.
#
- # - klass -> Deltacloud::Client::+Class+
+ # - name -> Deltacloud::Client::+Class+
+ # - error -> Deltacloud XML error representation
# - message -> Exception message (overiden by error body message if
# present)
- # - error -> Deltacloud XML error representation
#
def client_error(name, error, message=nil)
args = {
:message => message,
:status => error ? error[:status] : '500'
}
- # If Deltacloud API send error in response body, parse it.
- # Otherwise, when DC API send just plain text error, use
- # it as exception message.
+ # If Deltacloud API sends an error in the response body, parse it.
+ # Otherwise, when DC API sends just plain text error, use
+ # it as the exception message.
# If DC API does not send anything back, then fallback to
# the 'message' attribute.
#
@@ -52,7 +52,8 @@ module Deltacloud
@app.call(env).on_complete do |e|
case e[:status].to_s
when '401'
- raise client_error(:authentication_error, e, 'Invalid :api_user or :api_password')
+ raise client_error(:authentication_error, e,
+ 'Invalid :api_user or :api_password')
when '405'
raise client_error(
:invalid_state, e, 'Resource state does not permit this action'
--
1.7.11.7
