Overall, this looks, good, I am going to say mostly ACK, though a few comments here on things to be fixed/changed in later patch. There are also comments on next patch that will need to be fixed before pushing:
* The saving of template to the warehouse does not seem to happen anymore. This used to error our if you did not have the warehouse running, so I know it was doing something (though perhaps we don't want to prevent the saving of the template if warehouse is unavailable, just have some mechanism to retry later if unsuccessful). * This is my fault for not passing it along to you, but after talking to Ian M, he is expecting the os/version/arch info in either of the following two forms, so this will need to be tweaked a bit: <os> <name>Fedora</name> <version>13</version> <architecture>64-bit</architecture> </os> or: <os name="Fedora" version="13" architecture="64-bit"/> Leaning toward the 2nd option, unless it adds significant work over the other option. On Fri, 2010-09-24 at 23:21 +0200, [email protected] wrote: > From: Jan Provaznik <[email protected]> > > --- > src/app/models/image.rb | 4 +- > src/app/models/template.rb | 23 +++++++++++++++++--- > src/db/migrate/20100830150014_create_templates.rb | 3 ++ > 3 files changed, 24 insertions(+), 6 deletions(-) > > diff --git a/src/app/models/image.rb b/src/app/models/image.rb > index d4f4fa4..75cd9a6 100644 > --- a/src/app/models/image.rb > +++ b/src/app/models/image.rb > @@ -27,7 +27,7 @@ class Image < ActiveRecord::Base > cattr_reader :per_page > @@per_page = 15 > > - belongs_to :template > + belongs_to :template, :counter_cache => true > has_many :replicated_images, :dependent => :destroy > has_many :providers, :through => :replicated_images > > @@ -45,7 +45,7 @@ class Image < ActiveRecord::Base > STATE_COMPLETE = 'complete' > STATE_CANCELED = 'canceled' > > - ACTIVE_STATES = [ STATE_WAITING, STATE_BUILDING ] > + ACTIVE_STATES = [ STATE_QUEUED, STATE_WAITING, STATE_BUILDING ] > > def self.new_if_not_exists(data) > unless find_by_template_id(data[:template_id], :conditions => {:target > => data[:target]}) > diff --git a/src/app/models/template.rb b/src/app/models/template.rb > index 239de4d..2f33f40 100644 > --- a/src/app/models/template.rb > +++ b/src/app/models/template.rb > @@ -12,14 +12,18 @@ class Template < ActiveRecord::Base > # uncomment this after reworking view (currently is used wizard, > # so there can be situation when save is called and name and platform can > be > # unset) > - #validates_presence_of :name > - #validates_presence_of :platform > + validates_presence_of :name > + validates_presence_of :platform > + validates_presence_of :platform_version > + validates_presence_of :architecture > > def update_xml_attributes!(opts = {}) > doc = xml > doc.name = opts[:name] if opts[:name] > doc.platform = opts[:platform] if opts[:platform] > doc.description = opts[:description] if opts[:description] > + doc.platform_version = opts[:platform_version] if opts[:platform_version] > + doc.architecture = opts[:architecture] if opts[:architecture] > doc.services = (opts[:services] || []) if opts[:services] or > opts[:set_services] > doc.packages = (opts[:packages] || []) if opts[:packages] or > opts[:set_packages] > save_xml! > @@ -28,7 +32,8 @@ class Template < ActiveRecord::Base > def save_xml! > self[:xml] = xml.to_xml > @xml = nil > - save! > + set_uuid > + save_without_validation! > end > > def xml > @@ -48,10 +53,12 @@ class Template < ActiveRecord::Base > > def update_attrs > # TODO: generate real uuid here, e.g. with some ruby uuid generator > - self.uuid ||= "#{xml.name}-#{Time.now.to_f.to_s}" > + set_uuid > self.name = xml.name > self.summary = xml.description > self.platform = xml.platform > + self.platform_version = xml.platform_version > + self.architecture = xml.architecture > end > > def providers > @@ -61,4 +68,12 @@ class Template < ActiveRecord::Base > :conditions => {:images => {:template_id => self.id}} > ).map {|p| p.provider} > end > + > + def set_uuid > + self.uuid ||= "#{xml.name}-#{Time.now.to_f.to_s}" > + end > + > + def self.find_or_create(id) > + id ? Template.find(id) : Template.new > + end > end > diff --git a/src/db/migrate/20100830150014_create_templates.rb > b/src/db/migrate/20100830150014_create_templates.rb > index 00b1966..d23f5ec 100644 > --- a/src/db/migrate/20100830150014_create_templates.rb > +++ b/src/db/migrate/20100830150014_create_templates.rb > @@ -6,8 +6,11 @@ class CreateTemplates < ActiveRecord::Migration > t.string :uri > t.string :name > t.string :platform > + t.string :platform_version > + t.string :architecture > t.text :summary > t.boolean :complete, :default => false > + t.integer :images_count > t.timestamps > end > end _______________________________________________ deltacloud-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/deltacloud-devel
