From 1746642a44f133ae49668be2bba20a95ba8ae097 Mon Sep 17 00:00:00 2001 From: Thomas Linkin Date: Wed, 30 Nov 2016 13:40:31 -0500 Subject: [PATCH 1/7] Add new formatter for parameter differences This commit adds a new, more intelligent formatter method for consuming identified parameter differences between the catalogs. This formatter tries to show values side by side for simple comparison. This formatter is a basic first attempt and will be enhanced later. --- lib/puppet/catalog-diff/formater.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/lib/puppet/catalog-diff/formater.rb b/lib/puppet/catalog-diff/formater.rb index 7cff1a7..e874879 100644 --- a/lib/puppet/catalog-diff/formater.rb +++ b/lib/puppet/catalog-diff/formater.rb @@ -141,5 +141,24 @@ def render_pull(output) end end.join("\n") + "#{self.node_summary_header("#{output[:failed_nodes_total]} out of #{output[:total_nodes]} nodes failed to compile. failure rate:",output,:total_percentage)}" end + + def param_diff(resource_list, old, new) + results = resource_list.collect do |resource| + output = String.new + old_resource = old[resource] + new_resource = new[resource] + + params = Set.new + params.merge old_resource.keys + params.merge new_resource.keys + + output << "\t\033[1m#{header.to_s.gsub("_"," ").capitalize}\033[0m:\n" + params.each do |param| + output << "\t\tOld Value: #{old_resource[param]}\n" + output << "\t\tNew Value: #{new_resource[param]}\n" + output << "\n" + end + end.join("\n") + end end end From b8813fb89681404377c8a97c9e26c971695bb6fe Mon Sep 17 00:00:00 2001 From: Thomas Linkin Date: Wed, 30 Nov 2016 13:45:30 -0500 Subject: [PATCH 2/7] Enhance differ for Puppet 4 Without this commit, the "differ" was not using functional routines that would correctly consume catalogs from disk. Methods like `from_pson` are no longer available. The "differ" also has implicit code for reading '.json' extensions despite not having it documented that they were valid. This commit removes the ability to attempt to handle JSON catalogs. When it comes to PSON catalogs, it will use the appropriate and verified working method available in Puppet 4. This commit also sorts some of the results of the "diffing" process for cleaner presentation later. --- lib/puppet/catalog-diff/differ.rb | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/lib/puppet/catalog-diff/differ.rb b/lib/puppet/catalog-diff/differ.rb index c21a50a..272824b 100644 --- a/lib/puppet/catalog-diff/differ.rb +++ b/lib/puppet/catalog-diff/differ.rb @@ -39,23 +39,9 @@ def diff(options = {}) when '.marshal' tmp = Marshal.load(File.read(r)) when '.pson' - tmp = PSON.load(File.read(r)) - unless tmp.respond_to? :version - if Puppet::Resource::Catalog.respond_to? :from_data_hash - tmp = Puppet::Resource::Catalog.from_data_hash tmp - else - # The method was renamed in 3.5.0 - tmp = Puppet::Resource::Catalog.from_pson tmp - end - end - when '.json' - if Puppet::Resource::Catalog.respond_to? :from_data_hash - tmp = Puppet::Resource::Catalog.from_data_hash JSON.load(File.read(r)) - else - # The method was renamed in 3.5.0 - tmp = Puppet::Resource::Catalog.from_pson JSON.load(File.read(r)) - end - else + raw_content = File.read(r) + tmp = Puppet::Resource::Catalog.convert_from(:pson, raw_content) + else raise "Provide catalog with the appropriate file extension, valid extensions are pson, yaml and marshal" end @@ -88,8 +74,8 @@ def diff(options = {}) resource_diffs_titles = return_resource_diffs(titles[:to], titles[:from]) - output[:only_in_old] = resource_diffs_titles[:titles_only_in_old] - output[:only_in_new] = resource_diffs_titles[:titles_only_in_new] + output[:only_in_old] = resource_diffs_titles[:titles_only_in_old].sort + output[:only_in_new] = resource_diffs_titles[:titles_only_in_new].sort resource_diffs = compare_resources(from,to,options) output[:differences_in_old] = resource_diffs[:old] From 216b04d9d674596a5a1f54fdb4db164ac62dca83 Mon Sep 17 00:00:00 2001 From: Thomas Linkin Date: Wed, 30 Nov 2016 13:52:58 -0500 Subject: [PATCH 3/7] Clean up "diff" face, present cleaner data Without this commit, the "diff" face had a few different problems. The simple ones were a matter of unreadable code. The more important issue was the lazy method by which data coming out of the "differ" was formatted and presented. This commit starts by re-tabbing the whole document. The lines trying to output to the console were also difficult to read and a bit of a mess. They were split out into a more usable form. This commit also takes advantage of the new method of presenting parameter differences that has been added to the formatter. --- lib/puppet/face/catalog/diff.rb | 61 +++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 26 deletions(-) diff --git a/lib/puppet/face/catalog/diff.rb b/lib/puppet/face/catalog/diff.rb index d2f6499..575c158 100644 --- a/lib/puppet/face/catalog/diff.rb +++ b/lib/puppet/face/catalog/diff.rb @@ -162,11 +162,11 @@ with_changes = nodes.select { |node,summary| summary.is_a?(Hash) && !summary[:node_percentage].zero? } most_changed = with_changes.sort_by {|node,summary| summary[:node_percentage]}.map do |node,summary| - Hash[node => summary[:node_percentage]] + Hash[node => summary[:node_percentage]] end most_differences = with_changes.sort_by {|node,summary| summary[:node_differences]}.map do |node,summary| - Hash[node => summary[:node_differences]] + Hash[node => summary[:node_differences]] end total_nodes = nodes.size nodes[:total_percentage] = (nodes.collect{|node,summary| summary.is_a?(Hash) && summary[:node_percentage] || nil }.compact.inject{|sum,x| sum.to_f + x } / total_nodes) @@ -183,34 +183,43 @@ format = Puppet::CatalogDiff::Formater.new() nodes.collect do |node,summary| - next if node == :total_percentage or node == :total_nodes or node == :most_changed or node == :with_changes or node == :most_differences or node == :pull_output or node == :date - format.node_summary_header(node,summary,:node_percentage) + summary.collect do |header,value| - next if value.nil? - if value.is_a?(Hash) - value.collect do |resource_id,resource| - next if resource.nil? - if resource.is_a?(Hash) && resource.has_key?(:type) - # If we find an actual resource print it out - format.resource_reference(header,resource_id,resource) - elsif resource.is_a?(Array) - next unless resource.any? - # Format string diffs - format.string_diff(header,resource_id,resource) - else + next if [:params_in_new, :params_in_old, :total_percentage, :total_nodes, :most_changed, :with_changes, :most_differences, :pull_output, :date].include? node + report = format.node_summary_header(node,summary,:node_percentage) + + report << summary.collect do |header,value| + next if value.nil? + if value.is_a?(Hash) + value.collect do |resource_id,resource| next if resource.nil? - # Format hash diffs - format.params_diff(header,resource_id,resource) + if resource.is_a?(Hash) && resource.has_key?(:type) + # If we find an actual resource print it out + format.resource_reference(header,resource_id,resource) + elsif resource.is_a?(Array) + next unless resource.any? + # Format string diffs + format.string_diff(header,resource_id,resource) + else + next if resource.nil? + # Format hash diffs + format.params_diff(header,resource_id,resource) + end end + elsif value.is_a?(Array) + next if value.empty? + # Format arrays + format.list(header,value) + else + format.key_pair(header,value) end - elsif value.is_a?(Array) - next if value.empty? - # Format arrays - format.list(header,value) - else - format.key_pair(header,value) - end end.delete_if {|x| x.nil? or x == [] }.join("\n") - end.join("\n") + "#{format.node_summary_header("#{nodes[:with_changes]} out of #{nodes[:total_nodes]} nodes changed.",nodes,:total_percentage)}\n#{format.list_hash("Nodes with the most changes by percent changed",nodes[:most_changed])}\n\n#{format.list_hash("Nodes with the most changes by differeces",nodes[:most_differences],'')}#{(nodes.has_key?(:pull_output) && format.render_pull(nodes[:pull_output]))}" + + report << "\033Parameter differences across catalogs:\033[0m:\n" + report << format.param_diff(node[:params_in_new.keys, node[:params_in_old], node[:params_in_new]) + end.join("\n") + + report << "#{format.node_summary_header("#{nodes[:with_changes]} out of #{nodes[:total_nodes]} nodes changed.",nodes,:total_percentage)}\n" + report << "#{format.list_hash("Nodes with the most changes by percent changed",nodes[:most_changed])}\n\n" + report << "#{format.list_hash("Nodes with the most changes by differeces",nodes[:most_differences],'')}#{(nodes.has_key?(:pull_output) && format.render_pull(nodes[:pull_output]))}" end end end From b2a75d7f727d2f55c38a9c82245af62efd891d51 Mon Sep 17 00:00:00 2001 From: Thomas Linkin Date: Thu, 1 Dec 2016 10:34:37 -0500 Subject: [PATCH 4/7] Run the param diff formatting correctly The code that sends the differ output off to be formatted takes a somewhat simple approach of treating data generically. To present the param diff data in a more usable manner, a purpose specific method has been created. We need to make sure this method gets used instead of the generic formatting approach for this data. Right now the approach taken for keeping that data from going through the generic method is wrong. A lack of understanding since the code is sort of dense. This commit ensures that the param data not be passed through the generic formatting loop. It also ensures that the param data that gets passed is actually coming from the correct place. --- lib/puppet/face/catalog/diff.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/puppet/face/catalog/diff.rb b/lib/puppet/face/catalog/diff.rb index 575c158..9393dc4 100644 --- a/lib/puppet/face/catalog/diff.rb +++ b/lib/puppet/face/catalog/diff.rb @@ -182,12 +182,14 @@ require File.expand_path(File.join(File.dirname(__FILE__), "..", "..", "catalog-diff", "formater.rb")) format = Puppet::CatalogDiff::Formater.new() + report = String.new + nodes.collect do |node,summary| - next if [:params_in_new, :params_in_old, :total_percentage, :total_nodes, :most_changed, :with_changes, :most_differences, :pull_output, :date].include? node - report = format.node_summary_header(node,summary,:node_percentage) + next if [:total_percentage, :total_nodes, :most_changed, :with_changes, :most_differences, :pull_output, :date].include? node + report << format.node_summary_header(node,summary,:node_percentage) report << summary.collect do |header,value| - next if value.nil? + next if ([:params_in_new, :params_in_old].include? header) or value.nil? if value.is_a?(Hash) value.collect do |resource_id,resource| next if resource.nil? @@ -214,7 +216,7 @@ end.delete_if {|x| x.nil? or x == [] }.join("\n") report << "\033Parameter differences across catalogs:\033[0m:\n" - report << format.param_diff(node[:params_in_new.keys, node[:params_in_old], node[:params_in_new]) + report << format.param_diff(summary[:params_in_new].keys, summary[:params_in_old], summary[:params_in_new]) end.join("\n") report << "#{format.node_summary_header("#{nodes[:with_changes]} out of #{nodes[:total_nodes]} nodes changed.",nodes,:total_percentage)}\n" From af00ebd0a7601ee3dbfa19952f3a5f33beeeb736 Mon Sep 17 00:00:00 2001 From: Thomas Linkin Date: Thu, 1 Dec 2016 11:14:58 -0500 Subject: [PATCH 5/7] Correct the param diff formatter This commit ensures that the output generated by the param diff formatter. This commit also corrects an issue where the name of the resource in question was not being passed correctly. --- lib/puppet/catalog-diff/formater.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/puppet/catalog-diff/formater.rb b/lib/puppet/catalog-diff/formater.rb index e874879..bc1a6aa 100644 --- a/lib/puppet/catalog-diff/formater.rb +++ b/lib/puppet/catalog-diff/formater.rb @@ -152,13 +152,15 @@ def param_diff(resource_list, old, new) params.merge old_resource.keys params.merge new_resource.keys - output << "\t\033[1m#{header.to_s.gsub("_"," ").capitalize}\033[0m:\n" + output << "\t\033[1m#{resource}\033[0m:\n" params.each do |param| - output << "\t\tOld Value: #{old_resource[param]}\n" - output << "\t\tNew Value: #{new_resource[param]}\n" + output << "\t\tOld Value: #{old_resource[param] || "UNDEF"}\n" + output << "\t\tNew Value: #{new_resource[param] || "UNDEF"}\n" output << "\n" end - end.join("\n") + + output + end.join end end end From a84bd8f4ac203e697cebdcba6eef3a3414d5fbdc Mon Sep 17 00:00:00 2001 From: Thomas Linkin Date: Tue, 6 Dec 2016 10:32:32 -0500 Subject: [PATCH 6/7] Resource diff format enabled by default This commit enables the presentation of resources in "diff" format as a default. The command line flag "display_resource_diff" has been changed to "no_resource_diff" to be used to disable the behavior. --- lib/puppet/catalog-diff/comparer.rb | 2 +- lib/puppet/face/catalog/diff.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/puppet/catalog-diff/comparer.rb b/lib/puppet/catalog-diff/comparer.rb index 1ce6318..391d69c 100644 --- a/lib/puppet/catalog-diff/comparer.rb +++ b/lib/puppet/catalog-diff/comparer.rb @@ -46,7 +46,7 @@ def compare_resources(old, new, options) parameters_in_new[resource[:resource_id]] = \ Hash[(new_resource[:parameters].to_a - resource[:parameters].to_a )] - if options[:show_resource_diff] + unless options[:no_resource_diff] Puppet.debug("Resource diff: #{resource[:resource_id]}") diff_array = str_diff( diff --git a/lib/puppet/face/catalog/diff.rb b/lib/puppet/face/catalog/diff.rb index 9393dc4..bf0106e 100644 --- a/lib/puppet/face/catalog/diff.rb +++ b/lib/puppet/face/catalog/diff.rb @@ -29,8 +29,8 @@ summary "Whether to show a diff for File resource content" end - option '--show_resource_diff' do - summary 'Display differeces between resources in unified diff format' + option '--no_resource_diff' do + summary 'Do not display differeces between resources in unified diff format' end option '--exclude_classes' do From e60c0245a1e9ddce97b31ab58b9fa266df3179b6 Mon Sep 17 00:00:00 2001 From: Thomas Linkin Date: Wed, 4 Jan 2017 15:46:22 -0500 Subject: [PATCH 7/7] Load PSON properly between versions of Puppet This commit adds back the code for loading PSON files and JSON files in Puppet 3.x. It also ensures that the correct loading method is used between 3.x and 4.x versions of Puppet. In Puppet 4.x it raises an error when trying to load JSON catalogs. There was not an obvious method for loading these files that could be found, additionally a JSON catalog could not be found to test with. --- lib/puppet/catalog-diff/differ.rb | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/lib/puppet/catalog-diff/differ.rb b/lib/puppet/catalog-diff/differ.rb index 272824b..d440905 100644 --- a/lib/puppet/catalog-diff/differ.rb +++ b/lib/puppet/catalog-diff/differ.rb @@ -39,8 +39,34 @@ def diff(options = {}) when '.marshal' tmp = Marshal.load(File.read(r)) when '.pson' - raw_content = File.read(r) - tmp = Puppet::Resource::Catalog.convert_from(:pson, raw_content) + if Puppet::Util::Package.versioncmp(Puppet.version, '4.0.0') < 0 + # If we're still in Puppet 3.X, use the 3.X loading method + tmp = PSON.load(File.read(r)) + unless tmp.respond_to? :version + if Puppet::Resource::Catalog.respond_to? :from_data_hash + tmp = Puppet::Resource::Catalog.from_data_hash tmp + else + # The method was renamed in 3.5.0 + tmp = Puppet::Resource::Catalog.from_pson tmp + end + end + else + + # Puppet 4 Method of loading the pson is quite different than 3.X + raw_content = File.read(r) + tmp = Puppet::Resource::Catalog.convert_from(:pson, raw_content) + end + when '.json' + if Puppet::Util::Package.versioncmp(Puppet.version, '4.0.0') <= 0 + raise "Loading JSON files will not work when using puppet-diff on Puppet 4.X" + end + + if Puppet::Resource::Catalog.respond_to? :from_data_hash + tmp = Puppet::Resource::Catalog.from_data_hash JSON.load(File.read(r)) + else + # The method was renamed in 3.5.0 + tmp = Puppet::Resource::Catalog.from_pson JSON.load(File.read(r)) + end else raise "Provide catalog with the appropriate file extension, valid extensions are pson, yaml and marshal" end