One minor suggestion/question below. Other than that and other minor points in previous responses, ACK to the whole series, everything seems to be working as it should.
On Wed, Feb 17, 2010 at 4:16 PM, Scott Seago sseago@redhat.com wrote:
Signed-off-by: Scott Seago sseago@redhat.com
<snip>
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb index 83d463a..a528e4e 100644 --- a/src/app/models/instance.rb +++ b/src/app/models/instance.rb @@ -39,9 +39,9 @@ class Instance < ActiveRecord::Base validates_presence_of :name validates_uniqueness_of :name, :scope => :portal_pool_id
- # FIXME: for now, flavor is required, realm is optional, although for RHEV-M,
- # flavor may be optional too
- validates_presence_of :flavor_id
- # FIXME: for now, hardware profile is required, realm is optional, although for RHEV-M,
- # hardware profile may be optional too
- validates_presence_of :hardware_profile_id
validates_presence_of :image_id
STATE_NEW = "new" @@ -83,5 +83,24 @@ class Instance < ActiveRecord::Base return task end
- def front_end_realm
Two questions here: 1. Do we need to return ""? Could we just not set anything? 2a. If we do, why not phrase this as: return "" if cloud_account.nil? <build string stuff> 2b. If it is not needed (which is my thought), perhaps phrase as: unless cloud_account.nil? <build string stuff>
- if cloud_account.nil?
- ""
- else
- cloud_account.provider.name + Realm::AGGREGATOR_REALM_PROVIDER_DELIMITER +
- cloud_account.username + (realm.nil? ? "" :
- (Realm::AGGREGATOR_REALM_ACCOUNT_DELIMITER +
- realm.name))
- end
- end
<snip>