Comments in-line. -- Jacob Helwig
On Fri, 15 Oct 2010 17:16:12 -0700, Jacob Helwig wrote: > > From: Nick Lewis <[email protected]> > > Collects parameters and their sources for nodes/groups and shows > them when viewing the node/group. Also refactors views to extract > common functionality into partials, for viewing lists of classes, > groups and parameters. > > Signed-off-by: Nick Lewis <[email protected]> > --- > app/models/node.rb | 38 ++------------------ > app/views/nodes/_nodes.html.haml | 13 ++++--- > app/views/nodes/show.html.haml | 30 ++--------------- > app/views/shared/_classes.html.haml | 17 +++++++++ > app/views/shared/_groups.html.haml | 17 +++++++++ > app/views/shared/_parameters.html.haml | 26 ++++++++++++++ > lib/node_group_graph.rb | 55 ++++++++++++++++++++++++----- > spec/models/node_spec.rb | 58 > ++++++++++++++++---------------- > 8 files changed, 149 insertions(+), 105 deletions(-) > create mode 100644 app/views/shared/_classes.html.haml > create mode 100644 app/views/shared/_groups.html.haml > create mode 100644 app/views/shared/_parameters.html.haml > > diff --git a/app/models/node.rb b/app/models/node.rb > index dc6de11..299bad8 100644 > --- a/app/models/node.rb > +++ b/app/models/node.rb > @@ -107,7 +107,7 @@ class Node < ActiveRecord::Base > end > > def configuration > - { 'name' => name, 'classes' => all_classes.collect(&:name), 'parameters' > => compiled_parameters } > + { 'name' => name, 'classes' => all_classes.collect(&:name), 'parameters' > => parameter_list } > end > > def to_yaml(opts={}) > @@ -118,40 +118,10 @@ class Node < ActiveRecord::Base > TimelineEvent.for_node(self) > end > > - # This wrapper method is just used to cache the result of the recursive > method > - def compiled_parameters(allow_conflicts=false) > - unless @compiled_parameters > - @compiled_parameters, @conflicts = compile_subgraph_parameters(self, > node_group_graph) > - @conflicts.each do |key| > - errors.add(:parameters,key) > - end > - end > - raise ParameterConflictError unless allow_conflicts or @conflicts.empty? > - @compiled_parameters > - end > + # Placeholder attributes > > - # Walks the graph of node groups for the given node, compiling parameters > by > - # merging down (preferring parameters specified in node groups that are > - # nearer). Raises a ParameterConflictError if parameters at the same > distance > - # from the node have the same name. > - def compile_subgraph_parameters(group,subgraph) > - children = subgraph.map do |child,child_subgraph| > - compile_subgraph_parameters(child,child_subgraph) > - end > - # Pick-up conflicts that our children had > - conflicts = children.map(&:last).inject(Set.new,&:merge) > - params = group.parameters.to_hash > - inherited = {} > - # Now collect our inherited params and their conflicts > - children.map(&:first).map {|h| [*h]}.flatten.each_slice(2) do |key,value| > - conflicts.add(key) if inherited[key] && inherited[key] != value > - inherited[key] = value > - end > - # Resolve all possible conflicts > - conflicts.each do |key| > - conflicts.delete(key) if params[key] > - end > - [params.reverse_merge(inherited), conflicts] > + def environment > + 'production' > end Looks like the added lines here were removed in eeac9ee. Bad rebase? > > def status_class > diff --git a/app/views/nodes/_nodes.html.haml > b/app/views/nodes/_nodes.html.haml > index 3d8b8b2..9636314 100644 > --- a/app/views/nodes/_nodes.html.haml > +++ b/app/views/nodes/_nodes.html.haml > @@ -1,27 +1,30 @@ > - nodes = paginate_scope(local_assigns[:nodes]) > - more_link = local_assigns[:more_link] > +- container = local_assigns[:container] Is up in the previous commit, but it looks like this was starting to be used before it was actually added to the partial? > > %table.main > %thead > %tr > - -# %th.check > - -# = check_box_tag "check_all" > %th.status > %th.hostname > Hostname > + - unless container.nil? > + %th Source groups > %th.latest_report.desc > ↓ Latest report > %tbody > - if nodes.present? > - - for node in nodes > + - nodes.each do |node| > + - sources = container.node_list[node] unless container.nil? > %tr[node]{:class => "#{'active' if node == @node}"} > - -# %td.check > - -# = check_box_tag dom_id(node) > %td.status{:class => node.status_class} > %span{:title => node_title_text(node)} > = node_status_icon(node) > %td.hostname > = link_to h(node.name), node > + - unless container.nil? > + %td > + = sources.map{|s| link_to(s.name,s)}.join(", ") unless > sources.include?(container) > %td.latest_report > = node.last_report ? node.last_report.time : "Has not reported" > = pagination_for nodes, more_link > diff --git a/app/views/nodes/show.html.haml b/app/views/nodes/show.html.haml > index f630c70..10bc3c0 100644 > --- a/app/views/nodes/show.html.haml > +++ b/app/views/nodes/show.html.haml > @@ -12,33 +12,9 @@ > - unless @node.description.blank? > .description= simple_format h(@node.description) > > - -# Conflicting parameters are okay here, since we just want to warn > - - if @node.compiled_parameters(true).present? > - - if @node.errors.on(:parameters) > - %p.error > - %strong Warning: > - The following parameters have multiple conflicting values: > - = [[email protected](:parameters)].join(", ") > - = inspector_table @node.compiled_parameters(true), :key, :value, :link > => false, :caption => 'Parameters' > - > - > - .section.half > - %h3 Groups > - - unless @node.node_groups.empty? > - = inspector_table @node.node_groups, :name, false, :link => true > - - else > - = describe_no_matches_for :groups > - > - .section.half > - %h3 Classes > - - unless @node.node_classes.empty? > - = inspector_table @node.node_classes, :name, false, :link => true > - - else > - = describe_no_matches_for :classes > - > - - unless @node.inherited_classes.empty? > - = inspector_table @node.inherited_classes, :name, false, :link => > true, :caption => 'Inherited Classes' > - > + = render 'shared/parameters', :resource => @node > + = render 'shared/groups', :resource => @node > + = render 'shared/classes', :resource => @node > > %br.clear > > diff --git a/app/views/shared/_classes.html.haml > b/app/views/shared/_classes.html.haml > new file mode 100644 > index 0000000..6a7599b > --- /dev/null > +++ b/app/views/shared/_classes.html.haml > @@ -0,0 +1,17 @@ > +.section.half > + %h3 Classes > + - unless resource.node_class_list.empty? > + %table.inspector > + %thead > + %tr > + %th.name Class > + %th Source groups > + %tbody > + - resource.node_class_list.each do |node_class,parents| > + %tr > + %td.name > + %strong= link_to(node_class.name,node_class) > + %td > + = parents.map{|p| link_to(p.name,p)}.join(", ") unless > parents.include?(resource) > + - else > + = describe_no_matches_as 'No classes' > diff --git a/app/views/shared/_groups.html.haml > b/app/views/shared/_groups.html.haml > new file mode 100644 > index 0000000..1d22793 > --- /dev/null > +++ b/app/views/shared/_groups.html.haml > @@ -0,0 +1,17 @@ > +.section.half > + %h3 Groups > + - unless resource.node_group_list.empty? > + %table.inspector > + %thead > + %tr > + %th Group > + %th Source groups > + %tbody > + - resource.node_group_list.each do |group,parents| > + %tr > + %td > + %strong= link_to(group.name,group) > + %td > + = parents.map{|p| link_to(p.name,p)}.join(", ") unless > parents.include?(resource) > + - else > + = describe_no_matches_as 'No groups' > diff --git a/app/views/shared/_parameters.html.haml > b/app/views/shared/_parameters.html.haml > new file mode 100644 > index 0000000..89765c0 > --- /dev/null > +++ b/app/views/shared/_parameters.html.haml > @@ -0,0 +1,26 @@ > +.section > + %h3 Parameters > + - # Conflicting parameters are okay here, since we just want to warn > + - unless resource.compiled_parameters(true).empty? > + - if resource.errors.on(:parameters) > + %p.error > + %strong Warning: > + The following parameters have multiple conflicting values: > + = [*resource.errors.on(:parameters)].join(", ") > + %table.inspector > + %thead > + %tr > + %th Key > + %th Value > + %th Source groups > + %tbody > + - resource.compiled_parameters(true).each do |param| > + %tr > + %td{:style => ("background-color: pink;" if sources.size > 1)} > + %tt= param.name > + %td > + %tt= param.value > + %td > + = param.sources.map{|source| > link_to(source.name,node_group_path(source.id))}.join(", ") unless > param.sources == Set[resource] > + - else > + = describe_no_matches_as 'No parameters' > diff --git a/lib/node_group_graph.rb b/lib/node_group_graph.rb > index 88fbc48..8b66112 100644 > --- a/lib/node_group_graph.rb > +++ b/lib/node_group_graph.rb > @@ -1,3 +1,5 @@ > +require 'ostruct' > + > module NodeGroupGraph > # Returns a hash of all the groups for this group/node, direct or > inherited. > # Each key is a group, and each value is the Set of groups from which we > inherit > @@ -30,6 +32,49 @@ module NodeGroupGraph > @node_class_list = all > end > > + # Collects all the parameters of the node, starting at the "most distant" > groups > + # and working its ways up to the node itself. If there is a conflict > between two > + # groups at the same level, the conflict is deferred to the next level. If > the > + # conflict reaches the top without being resolved, a > ParameterConflictError is > + # raised. > + def compiled_parameters(allow_conflicts=false) > + unless @compiled_parameters > + @compiled_parameters = self.walk_groups do |group,parents| > + # Pick-up conflicts that our parents had > + parent_params = parents.map(&:parameters).flatten > + conflicts = parents.map(&:conflicts).inject(Set.new,&:merge) > + > + params = {} > + group.parameters.to_hash.each do |key,value| > + params[key] = OpenStruct.new :name => key, :value => value, > :sources => Set[group] > + end > + > + #Now collect our inherited params and their conflicts > + inherited = {} > + parent_params.each do |parameter| > + if inherited[parameter.name] && inherited[parameter.name].value != > parameter.value > + conflicts.add(parameter.name) > + inherited[parameter.name].sources << parameter.sources.first > + else > + inherited[parameter.name] = OpenStruct.new :name => > parameter.name, :value => parameter.value, :sources => parameter.sources > + end > + end > + > + # Resolve all conflicts resolved by the node/group itself > + conflicts.delete_if {|key| params[key]} > + > + OpenStruct.new :parameters => > params.reverse_merge(inherited).values, :conflicts => conflicts > + end > + @compiled_parameters.conflicts.each { |key| > errors.add(:parameters,key) } > + end > + raise ParameterConflictError unless allow_conflicts or > @compiled_parameters.conflicts.empty? > + @compiled_parameters.parameters > + end > + > + def parameter_list > + compiled_parameters.map{|param| {param.name => param.value} > }.inject({},&:merge) > + end > + > def walk_groups(&block) > walk(:node_groups,&block) > end > @@ -38,17 +83,7 @@ module NodeGroupGraph > walk(:node_group_children,&block) > end > > - def node_group_graph > - @node_group_graph ||= compile_node_group_graph.last > - end > - > private > - def compile_node_group_graph(group=self, seen=[], all=[]) > - return [nil,{}] if seen.include? group > - all << group > - graph = group.node_groups.map {|grp| {grp => > compile_node_group_graph(grp, seen + [group], all).last}}.inject({},&:merge) > - [all.uniq, graph] > - end > > def walk(method,&block) > def yield_children(seen,method,&block) > diff --git a/spec/models/node_spec.rb b/spec/models/node_spec.rb > index 99283e0..de941d1 100644 > --- a/spec/models/node_spec.rb > +++ b/spec/models/node_spec.rb > @@ -197,8 +197,11 @@ describe Node do > end > > it "should return the node's compiled parameters in the returned > parameters list" do > - @node.stubs(:compiled_parameters).returns({'a' => 'b', 'c' => 'd'}) > - @node.configuration['parameters'].should == { 'a' => 'b', 'c' => 'd' } > > + @node.stubs(:compiled_parameters).returns [ > + OpenStruct.new(:name => 'a', :value => 'b', :sources => Set[:foo]), > + OpenStruct.new(:name => 'c', :value => 'd', :sources => Set[:bar]) > + ] > + @node.configuration['parameters'].should == { 'a' => 'b', 'c' => 'd' } > end > end > > @@ -280,39 +283,18 @@ describe Node do > @node_group_b.node_groups << @node_group_c > end > > - it "should return the correct graph" do > - @node.node_group_graph.should == { > - @node_group_a => { > - @node_group_c => {...@node_group_d => {}} > - }, > - @node_group_b => { > - @node_group_c => {...@node_group_d => {}} > - } > - } > - end > - > it "should return the correct list" do > @node.node_group_list.should == {...@node_group_a => s...@node], > @node_group_c => s...@node_group_a,@node_group_b], @node_group_b => > s...@node], @node_group_d => s...@node_group_c]} > end > end > > - it "should handle cycles gracefully" do > - NodeGroupEdge.new(:from => @node_group_a, :to => > @node_group_b).save(false) > - NodeGroupEdge.new(:from => @node_group_b, :to => > @node_group_a).save(false) > - > - @node.node_group_graph.should == { > - @node_group_a => { > - @node_group_b => { > - @node_group_a => {} }}, > - @node_group_b => { > - @node_group_a => { > - @node_group_b => {} }}} > - end > - > describe "handling parameters in the graph" do > > it "should return the compiled parameters" do > - @node.compiled_parameters.should == {'foo' => '1', 'bar' => '2'} > + @node.compiled_parameters.should == [ > + OpenStruct.new(:name => 'foo', :value => '1', :sources => > s...@node_group_a]), > + OpenStruct.new(:name => 'bar', :value => '2', :sources => > s...@node_group_b]) > + ] > end > > it "should ensure that parameters nearer to the node are retained" do > @@ -320,7 +302,10 @@ describe Node do > @node_group_a1.parameters << Parameter.create(:key => 'foo', :value > => '2') > @node_group_a.node_groups << @node_group_a1 > > - @node.compiled_parameters.should == {'foo' => '1', 'bar' => '2'} > + @node.compiled_parameters.should == [ > + OpenStruct.new(:name => 'foo', :value => '1', :sources => > s...@node_group_a]), > + OpenStruct.new(:name => 'bar', :value => '2', :sources => > s...@node_group_b]) > + ] > end > > it "should raise an error if there are parameter conflicts among > children" do > @@ -353,7 +338,22 @@ describe Node do > it "should include parameters of the node itself" do > @node.parameters << Parameter.create(:key => "node_parameter", > :value => "exist") > > - @node.compiled_parameters["node_parameter"].should == "exist" > + @node.compiled_parameters.first.name.should == "node_parameter" > + @node.compiled_parameters.first.value.should == "exist" > + end > + > + it "should retain the history of its parameters" do > + @node_group_c = NodeGroup.generate! :name => "C" > + @node_group_d = NodeGroup.generate! :name => "D" > + @node_group_c.parameters << Parameter.generate(:key => 'foo', :value > => '3') > + @node_group_d.parameters << Parameter.generate(:key => 'foo', :value > => '4') > + @node_group_a.node_groups << @node_group_c > + @node_group_a.node_groups << @node_group_d > + > + @node.compiled_parameters.should == [ > + OpenStruct.new(:name => 'foo', :value => '1', :sources => > s...@node_group_a]), > + OpenStruct.new(:name => 'bar', :value => '2', :sources => > s...@node_group_b]) > + ] > end > end > end > -- > 1.7.3.1 >
signature.asc
Description: Digital signature
