Merge pull request #244 from kpaulisse/kpaulisse-ignore-comparison-tag

Update capability and docs of --compare-file-text

Author: ahayworth

doc/advanced-compare-file-text.md

@@ -0,0 +1,79 @@
+# Compare file text
+
+`octocatalog-diff` contains functionality to detect changes in file content for file resources that use the `source` attribute like this:
+
+```
+file { '/etc/logrotate.d/my-service.conf':
+  source => 'puppet:///modules/logrotate/etc/logrotate.d/my-service.conf',
+}
+```
+
+When the `source` attribute is used, the catalog contains the value of the attribute (in the example above, `source` = `puppet:///modules/logrotate/etc/logrotate.d/my-service.conf`). However, the catalog does not contain the content of the file. When an agent applies the catalog, the file found on the server or in the code base at `modules/logrotate/file/etc/logrotate.d/my-service.conf` will be installed into `/etc/logrotate.d/my-service.conf`.
+
+If a developer creates a change to this file, the catalog will not indicate this change. This means that so long as the `source` location does not change, any tool analyzing just the catalog would not detect a "diff" resulting from changes in the underlying file itself.
+
+However, since applying the catalog could change the content of a file on the target node, `octocatalog-diff` has a feature that will do the following for any `source => 'puppet:///modules/...'` files:
+
+- Locate the source file in the Puppet code
+- Substitute the content of the file into the generated catalog (remove `source` and populate `content`)
+- Display the "diff" in the now-populated `content` field
+
+This feature is available only when the catalogs are being compiled from local code. This feature is not available, and will be automatically disabled, when pulling catalogs from PuppetDB or a Puppet server.
+
+Note: In Puppet >= 4.4 there is an option in Puppet itself called "static catalogs" which if enabled will cause the checksum of the file to be included in the catalog. However, the `octocatalog-diff` feature described here is still useful because it can be used to display a "diff" of the change rather than just displaying a "diff" of a checksum.
+## Command line options
+
+### `--compare-file-text` and `--no-compare-file-text`
+
+The feature described above is enabled by default, and no special setup is required.
+
+To disable this feature for a specific run, add `--no-compare-file-text` to the command line.
+
+To disable this feature by default, add the following to a [configuration](/doc/configuration.md) file:
+
+```ruby
+settings[:compare_file_text] = false
+```
+
+If this feature is disabled by default in a configuration file, add `--compare-file-text` to enable the feature for this specific run.
+
+Note that the feature will be automatically disabled, regardless of configuration or command line options, if catalogs are being pulled from PuppetDB or a Puppet server.
+
+### `--compare-file-text-ignore-tags`
+
+To disable this feature for specific `file` resources, set a tag on the resources for which the comparison is undesired. For example:
+
+```
+file { '/etc/logrotate.d/my-service.conf':
+  source => 'puppet:///modules/logrotate/etc/logrotate.d/my-service.conf',
+}
+
+file { '/etc/logrotate.d/other-service.conf':
+  tag    => ['compare-file-text-disable'],
+  source => 'puppet:///modules/logrotate/etc/logrotate.d/other-service.conf',
+}
+```
+
+Inform `octocatalog-diff` of the name of the tag either via the command line (`--compare-file-text-ignore-tags "compare-file-text-disable"`) or via a [configuration](/doc/configuration.md) file:
+
+```ruby
+settings[:compare_file_text_ignore_tags] = %w(compare-file-text-disable)
+```
+
+With this example setup, the file text would be compared for `/etc/logrotate.d/my-service.conf` but would NOT be compared for `/etc/logrotate.d/other-service.conf`.
+
+Notes:
+
+1. `--compare-file-text-ignore-tags` can take comma-separated arguments if there are multiple tags, e.g.: `--compare-file-text-ignore-tags tag1,tag2`.
+
+1. When defining values in the configuration file, `settings[:compare_file_text_ignore_tags]` must be an array. (The default value is an empty array, `[]`.)
+
+1. "compare-file-text-disable" is used as the name of the tag in the example above, but it is not a magical value. Any valid tag name can be used.
+
+## Notes
+
+1. If the underlying file source cannot be found when building the "to" catalog, an exception will be raised. This will display the resource type and title, the value of `source` that is invalid, and the file name and line number of the Puppet manifest that declared the resource.
+
+1. If the underlying file source cannot be found when building the "from" catalog, no exception will be raised. This behavior allows `octocatalog-diff` to work correctly even if there is a problem in the "from" catalog that is being corrected in the "to" catalog. (Of course, if the underlying file source can't be found in the "to" catalog either, an exception is raised -- see #1.)
+
+1. No processing takes place for file `source` whose values do not start with `puppet:///modules/`.

doc/optionsref.md

@@ -185,6 +185,8 @@ Usage: octocatalog-diff [command line options]
         --no-ignore-tags             Disable ignoring based on tags
         --ignore-tags STRING1[,STRING2[,...]]
                                      Specify tags to ignore
+        --compare-file-text-ignore-tags STRING1[,STRING2[,...]]
+                                     Tags that exclude a file resource from text comparison
         --[no-]preserve-environments Enable or disable environment preservation
         --environment STRING         Environment for catalog compilation globally
         --to-environment STRING      Environment for catalog compilation for the to branch
@@ -374,6 +376,21 @@ what is most often desired. (<a href="../lib/octocatalog-diff/cli/options/compar
     </td>
   </tr>
 
+  <tr>
+    <td valign=top>
+      <pre><code>--compare-file-text-ignore-tags STRING1[,STRING2[,...]]</code></pre>
+    </td>
+    <td valign=top>
+      Tags that exclude a file resource from text comparison
+    </td>
+    <td valign=top>
+      When a file is specified with `source => 'puppet:///modules/something/foo.txt'`, remove
+the 'source' attribute and populate the 'content' attribute with the text of the file.
+This allows for a diff of the content, rather than a diff of the location, which is
+what is most often desired. (<a href="../lib/octocatalog-diff/cli/options/compare_file_text.rb">compare_file_text.rb</a>)
+    </td>
+  </tr>
+
   <tr>
     <td valign=top>
       <pre><code>--create-symlinks STRING1[,STRING2[,...]]</code></pre>
@@ -1897,6 +1914,19 @@ has no effect when `--display-detail-add` is not used. (<a href="../lib/octocata
     </td>
   </tr>
 
+  <tr>
+    <td valign=top>
+      <pre><code>--use-lcs
+--no-use-lcs </code></pre>
+    </td>
+    <td valign=top>
+      Use the LCS algorithm to determine differences in arrays
+    </td>
+    <td valign=top>
+      Configures using the Longest common subsequence (LCS) algorithm to determine differences in arrays (<a href="../lib/octocatalog-diff/cli/options/use_lcs.rb">use_lcs.rb</a>)
+    </td>
+  </tr>
+
   <tr>
     <td valign=top>
       <pre><code>--validate-references

lib/octocatalog-diff/catalog-util/fileresources.rb

@@ -13,9 +13,16 @@ class FileResources
       # Public method: Convert file resources to text. See the description of the class
       # just above for details.
       # @param obj [OctocatalogDiff::Catalog] Catalog object (will be modified)
+      # @param environment [String] Environment (defaults to production)
       def self.convert_file_resources(obj, environment = 'production')
         return unless obj.valid? && obj.compilation_dir.is_a?(String) && !obj.compilation_dir.empty?
-        _convert_file_resources(obj.resources, obj.compilation_dir, environment)
+        _convert_file_resources(
+          obj.resources,
+          obj.compilation_dir,
+          environment,
+          obj.options[:compare_file_text_ignore_tags],
+          obj.options[:tag]
+        )
         begin
           obj.catalog_json = ::JSON.generate(obj.catalog)
         rescue ::JSON::GeneratorError => exc
@@ -70,8 +77,11 @@ def self.module_path(dir)
       # Internal method: Static method to convert file resources. The compilation directory is
       # required, or else this is a no-op. The passed-in array of resources is modified by this method.
       # @param resources [Array<Hash>] Array of catalog resources
-      # @param compilation_dir [String] Compilation directory (so files can be looked up)
-      def self._convert_file_resources(resources, compilation_dir, environment = 'production')
+      # @param compilation_dir [String] Compilation directory
+      # @param environment [String] Environment
+      # @param ignore_tags [Array<String>] Tags that exempt a resource from conversion
+      # @param to_from_tag [String] Either "to" or "from" for catalog type
+      def self._convert_file_resources(resources, compilation_dir, environment, ignore_tags, to_from_tag)
         # Calculate compilation directory. There is not explicit error checking here because
         # there is on-demand, explicit error checking for each file within the modification loop.
         return unless compilation_dir.is_a?(String) && compilation_dir != ''
@@ -88,38 +98,51 @@ def self._convert_file_resources(resources, compilation_dir, environment = 'prod
         resources.map! do |resource|
           if resource_convertible?(resource)
             path = file_path(resource['parameters']['source'], modulepaths)
-            if path.nil?
-              # Pass this through as a wrapped exception, because it's more likely to be something wrong
-              # in the catalog itself than it is to be a broken setup of octocatalog-diff.
-              message = "Errno::ENOENT: Unable to resolve '#{resource['parameters']['source']}'!"
-              raise OctocatalogDiff::Errors::CatalogError, message
-            end
 
-            if File.file?(path)
+            if resource['tags'] && ignore_tags && (resource['tags'] & ignore_tags).any?
+              # Resource tagged not to be converted -- do nothing.
+            elsif path && File.file?(path)
               # If the file is found, read its content. If the content is all ASCII, substitute it into
               # the 'content' parameter for easier comparison. If not, instead populate the md5sum.
               # Delete the 'source' attribute as well.
               content = File.read(path)
               is_ascii = content.force_encoding('UTF-8').ascii_only?
               resource['parameters']['content'] = is_ascii ? content : '{md5}' + Digest::MD5.hexdigest(content)
               resource['parameters'].delete('source')
-            elsif File.exist?(path)
+            elsif path && File.exist?(path)
               # We are not handling recursive file installs from a directory or anything else.
               # However, the fact that we found *something* at this location indicates that the catalog
               # is probably correct. Hence, the very general .exist? check.
+            elsif to_from_tag == 'from'
+              # Don't raise an exception for an invalid source in the "from"
+              # catalog, because the developer may be fixing this in the "to"
+              # catalog. If it's broken in the "to" catalog as well, the
+              # exception will be raised when this code runs on that catalog.
             else
-              # This is probably a bug
-              # :nocov:
-              raise "Unable to find '#{resource['parameters']['source']}' at #{path}!"
-              # :nocov:
+              # Pass this through as a wrapped exception, because it's more likely to be something wrong
+              # in the catalog itself than it is to be a broken setup of octocatalog-diff.
+              #
+              # Example error: <OctocatalogDiff::Errors::CatalogError: Unable to resolve
+              # source=>'puppet:///modules/test/tmp/bar' in File[/tmp/bar]
+              # (/x/modules/test/manifests/init.pp:46)>
+              source = resource['parameters']['source']
+              type = resource['type']
+              title = resource['title']
+              file = resource['file'].sub(Regexp.new('^' + Regexp.escape(env_dir) + '/'), '')
+              line = resource['line']
+              message = "Unable to resolve source=>'#{source}' in #{type}[#{title}] (#{file}:#{line})"
+              raise OctocatalogDiff::Errors::CatalogError, message
             end
           end
+
           resource
         end
       end
 
       # Internal method: Determine if a resource is convertible. It is convertible if it
       # is a file resource with no declared 'content' and with a declared and parseable 'source'.
+      # It is not convertible if the resource is tagged with one of the tags declared by
+      # the option `--compare-file-text-ignore-tags`.
       # @param resource [Hash] Resource to check
       # @return [Boolean] True of resource is convertible, false if not
       def self.resource_convertible?(resource)

lib/octocatalog-diff/cli/options/compare_file_text.rb

@@ -15,3 +15,21 @@ def parse(parser, options)
     end
   end
 end
+
+# Sometimes there is a particular file resource for which the file text
+# comparison is not desired, while not disabling that option globally. Similar
+# to --ignore_tags, it's possible to tag the file resource and exempt it from
+# the --compare_file_text checks.
+# @param parser [OptionParser object] The OptionParser argument
+# @param options [Hash] Options hash being constructed; this is modified in this method.
+OctocatalogDiff::Cli::Options::Option.newoption(:compare_file_text_ignore_tags) do
+  has_weight 415
+
+  def parse(parser, options)
+    description = 'Tags that exclude a file resource from text comparison'
+    parser.on('--compare-file-text-ignore-tags STRING1[,STRING2[,...]]', Array, description) do |x|
+      options[:compare_file_text_ignore_tags] ||= []
+      options[:compare_file_text_ignore_tags].concat x
+    end
+  end
+end

spec/octocatalog-diff/fixtures/catalogs/catalog-test-file-v4-tagged.json

@@ -0,0 +1,57 @@
+{
+  "document_type": "Catalog",
+  "tags": ["settings","test"],
+  "name": "my.rspec.node",
+  "version": "production",
+  "environment": "production",
+  "resources": [
+    {
+      "type": "Stage",
+      "title": "main",
+      "tags": ["stage"],
+      "exported": false,
+      "parameters": {
+        "name": "main"
+      }
+    },
+    {
+      "type": "Class",
+      "title": "Settings",
+      "tags": ["class","settings"],
+      "exported": false
+    },
+    {
+      "type": "File",
+      "title": "/tmp/foo",
+      "tags": ["file","class"],
+      "file": "/x/modules/test/manifests/init.pp",
+      "line": 37,
+      "exported": false,
+      "parameters": {
+        "backup": false,
+        "mode": "0440",
+        "owner": "root",
+        "group": "root",
+        "source": "puppet:///modules/test/tmp/foo"
+      }
+    },
+    {
+      "type": "File",
+      "title": "/tmp/bar",
+      "tags": ["file","class","no_compare_tag"],
+      "file": "/x/modules/test/manifests/init.pp",
+      "line": 46,
+      "exported": false,
+      "parameters": {
+        "backup": false,
+        "mode": "0440",
+        "owner": "root",
+        "group": "root",
+        "source": "puppet:///modules/test/tmp/bar"
+      }
+    }
+ ],
+  "classes": [
+    "test"
+  ]
+}

spec/octocatalog-diff/fixtures/repos/convert-resources/broken/modules/test/manifests/init.pp

@@ -1,9 +1,11 @@
 class test {
   file { '/tmp/foo1':
+    tag    => ['_convert_file_resources_foo1_'],
     source => 'puppet:///modules/test/foo-new',
   }
 
   file { '/tmp/foo2':
+    tag    => ['_convert_file_resources_foo2_'],
     source => 'puppet:///modules/test/foo-old',
   }
 }