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: Jacob Helwig <[email protected]> Signed-off-by: Nick Lewis <[email protected]> --- app/models/node.rb | 38 +-------------------- app/views/node_classes/show.html.haml | 2 +- app/views/node_groups/show.html.haml | 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 ++++++++++++++++---------------- 10 files changed, 166 insertions(+), 128 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..0ca6060 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,42 +118,6 @@ 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 - - # 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] - end - def status_class return 'no reports' unless last_report last_report.status diff --git a/app/views/node_classes/show.html.haml b/app/views/node_classes/show.html.haml index b6a035c..6a4ea50 100644 --- a/app/views/node_classes/show.html.haml +++ b/app/views/node_classes/show.html.haml @@ -15,6 +15,6 @@ .section %h3 Nodes in this class - if @node_class.nodes.present? - = render 'nodes/nodes', :nodes => @node_class.nodes + = render 'nodes/nodes', :nodes => @node_class.node_list.keys, :container => @node_class - else = describe_no_matches_for :nodes, :class diff --git a/app/views/node_groups/show.html.haml b/app/views/node_groups/show.html.haml index 9b7f084..acb2af2 100644 --- a/app/views/node_groups/show.html.haml +++ b/app/views/node_groups/show.html.haml @@ -9,27 +9,27 @@ %li= link_to 'Destroy', @node_group, :confirm => 'Are you sure?', :method => :delete, :class => "delete button" .item - .section - %h3 Parameters - - unless @node_group.parameters.blank? - = inspector_table @node_group.parameters.to_hash - - else - = describe_no_matches_as 'No parameters' - - .section.half - %h3 Inherited groups - - unless @node_group.node_groups.blank? - = inspector_table @node_group.node_groups, :name, false, :link => true - - else - = describe_no_matches_as 'No groups' + = render 'shared/parameters', :resource => @node_group + = render 'shared/groups', :resource => @node_group + = render 'shared/classes', :resource => @node_group .section.half - %h3 Classes - - unless @node_group.node_classes.blank? - = inspector_table @node_group.node_classes, :name, false, :link => true + %h3 Derived groups + - unless @node_group.node_group_child_list.empty? + %table.inspector + %thead + %tr + %th Group + %th Source groups + %tbody + - @node_group.node_group_child_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?(@node_group) - else - = describe_no_matches_as 'No classes' - + = describe_no_matches_as 'No derived groups' .item - if @node_group.nodes.present? .section @@ -37,6 +37,6 @@ .section %h3 Nodes for this group - if @node_group.nodes.present? - = render 'nodes/nodes', :nodes => @node_group.nodes + = render 'nodes/nodes', :nodes => @node_group.node_list.keys, :container => @node_group - else = describe_no_matches_for :nodes, :group 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] %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 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
