Jason Guiditta
2008-Apr-21 17:18 UTC
[Ovirt-devel] [Patch] refactor Pool models to use nested sets
On Mon, 2008-04-21 at 09:44 -0400, Scott Seago wrote:> Refactor Pool models (Hardware and VM) to implement the nested set > model. As part of this I've refactored the way the various daemon > processes include the model code and connect to the database, since we > also have to include the betternestedset plugin code. Now all non-rails > services that need to access the ovirt models just need to include > dutils/active_record_env.rb -- this will include the nested set stuff, > connect to the db, and include all of the ovirt models. > _______________________________________________ > Ovirt-devel mailing list > Ovirt-devel at redhat.com > https://www.redhat.com/mailman/listinfo/ovirt-develLooks fine to me overall, with a couple of thoughts/comments/questions: * In HardwareController + @parent = Pool.find(params[:parent_id]) + @perm_obj = @parent should we check for the param? What if it is null? looks like the next @perm_obj depends on it being there (this applies in multiple places actually) * Why reverse_each in Pool here rather than just each? + self_and_ancestors.reverse_each do |the_pool| * Why do we have some views that are .rthml and others that are .html.erb? diff --git a/wui/src/app/views/dashboard/index.html.erb b/wui/src/app/views/dashboard/index.html.erb * Is there is way we can specify this path w/o being absolute? Maybe a setable env var or maybe use gems for our resuable components? And what if the wui is on a different box than the daemons, since we were planning to split them out into their own rpm(I thought)? diff --git a/wui/src/dutils/active_record_env.rb b/wui/src/dutils/active_record_env.rb +$: << File.join(File.dirname(__FILE__), "../app") +$: << File.join(File.dirname(__FILE__), "../vendor/plugins/betternestedset/lib") +require '/usr/share/ovirt-wui/vendor/plugins/betternestedset/init.rb' + +def database_connect + $dbconfig YAML::load(ERB.new(IO.read('/usr/share/ovirt-wui/config/database.yml')).result) So I guess really only the first is a possible enhancement for right now, the rest are just things to consider.
Scott Seago
2008-Apr-21 17:38 UTC
[Ovirt-devel] [Patch] refactor Pool models to use nested sets
Jason Guiditta wrote:> On Mon, 2008-04-21 at 09:44 -0400, Scott Seago wrote: > >> Refactor Pool models (Hardware and VM) to implement the nested set >> model. As part of this I've refactored the way the various daemon >> processes include the model code and connect to the database, since we >> also have to include the betternestedset plugin code. Now all non-rails >> services that need to access the ovirt models just need to include >> dutils/active_record_env.rb -- this will include the nested set stuff, >> connect to the db, and include all of the ovirt models. >> _______________________________________________ >> Ovirt-devel mailing list >> Ovirt-devel at redhat.com >> https://www.redhat.com/mailman/listinfo/ovirt-devel >> > > Looks fine to me overall, with a couple of thoughts/comments/questions: > * In HardwareController > + @parent = Pool.find(params[:parent_id]) > + @perm_obj = @parent > should we check for the param? What if it is null? looks like the > next @perm_obj depends on it being there (this applies in multiple > places actually) >Yeah -- we probably should do this -- although as a general controller cleanup -- not part of this patch. There are various places that we assume a certain input parameter. Although the only way this param _won't_ be sent will be if the user is typing in random URLs -- we should provide a saner error message. if @parent is null here, then the error message will probably amount to trying to call permissions checks on a null obj, rather than "parent_id is required"> * Why reverse_each in Pool here rather than just each? > + self_and_ancestors.reverse_each do |the_pool| >Because we want to find the match closest to the current pool. i.e. if we're looking for the quota, we want the one for "this object or closest parent" rather than that of the root object. The self_and_ancestors method returns the root first.> * Why do we have some views that are .rthml and others that > are .html.erb? >Hmm. good question. All the initial views were rhtml -- but the rails 2.0 scaffolding generates .html.erb files. In fact, the templates we're using don't really differ based on the filename now. I think what happened was that when bclark generated new model templates, he kept those names. Or maybe it was me? In any case, it's mostly an artifact of generating rails scaffolding (that's since been mostly deleted) under different rails versions. Perhaps we should clean that up as part of the new html mockup integration.> * Is there is way we can specify this path w/o being absolute? Maybe a > setable env var or maybe use gems for our resuable components? And what > if the wui is on a different box than the daemons, since we were > planning to split them out into their own rpm(I thought)? >Hmm. Well I'm not sure if splitting off the rails models into a gem really fits into rails notion of where things should go. We could do a symlink there I guess. If it's just a matter of splitting off into separate RPMs -- that's more a packaging question than where things go. i.e. we can generate an ovirt-models RPM that includes everything in: /usr/share/ovirt-wui/vendor/plugins/betternestedset/ /usr/share/ovirt-wui/app/models/ /usr/share/ovirt-wui/app/util/ /usr/share/ovirt-wui/dutils/ Then ovirt-wui, ovirt-taskomatic, etc. can require this RPM and the current paths will still work. On the other hand, it's probably a better idea to put the Ruby search path stuff in the calling environment for taskomatic, etc. anyway -- which then avoids the problem of path location in the code itself.> So I guess really only the first is a possible enhancement for right > now, the rest are just things to consider. > >OK. so it sounds like we chould include the first thing above in a separate cleanup task for all the controllers and URL vars (unless it's better to fix this one instance now). Similarly the absolute path fixing needs to be done when we figure out how we're changing the way we run taskomatic, etc. Is there anything (above or otherwise) that I ought to modify in this patch before pushing it? Scott