Merge pull request #20 from github/kpaulisse-modulepath

Use modulepath from environment.conf if available

.version

@@ -1 +1 @@
-0.5.4
+0.5.6

doc/CHANGELOG.md

@@ -8,6 +8,22 @@
 </tr>
 </thead><tbody>
 <tr valign=top>
+<td>0.5.6</td>
+<td>2016-11-16</td>
+<td>
+<ul>
+<li><a href="https://github.com/github/octocatalog-diff/pull/20">https://github.com/github/octocatalog-diff/pull/20</a>: Use modulepath from environment.conf to inform lookup directories for <code>--compare-file-text</code> feature</li>
+</ul>
+</td>
+</tr>
+<tr valign=top>
+<td>0.5.5</td>
+<td>-</td>
+<td>
+Unreleased internal version
+</td>
+</tr>
+<tr valign=top>
 <td>0.5.4</td>
 <td>2016-11-07</td>
 <td>

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

@@ -21,6 +21,45 @@ def self.convert_file_resources(obj)
         end
       end
 
+      # Internal method: Locate a file that is referenced at puppet:///modules/xxx/yyy using the
+      # module path that is specified within the environment.conf file (assuming the default 'modules'
+      # directory doesn't exist or the module isn't found in there). If the file can't be found then
+      # this returns nil which may trigger an error.
+      # @param src [String] A file reference: puppet:///modules/xxx/yyy
+      # @param modulepaths [Array] Cached module path
+      # @return [String] File system path to referenced file
+      def self.file_path(src, modulepaths)
+        unless src =~ %r{^puppet:///modules/([^/]+)/(.+)}
+          raise ArgumentError, "Bad parameter source #{src}"
+        end
+
+        path = File.join(Regexp.last_match(1), 'files', Regexp.last_match(2))
+        modulepaths.each do |mp|
+          file = File.join(mp, path)
+          return file if File.exist?(file)
+        end
+
+        nil
+      end
+
+      # Internal method: Parse environment.conf to find the modulepath
+      # @param compilation_dir [String] Compilation directory
+      # @return [Array] Module paths
+      def self.module_path(compilation_dir)
+        environment_conf = File.join(compilation_dir, 'environment.conf')
+        unless File.file?(environment_conf)
+          return [File.join(compilation_dir, 'modules')]
+        end
+
+        # This doesn't support multi-line, continuations with backslash, etc.
+        # Does it need to??
+        if File.read(environment_conf) =~ /^modulepath\s*=\s*(.+)/
+          Regexp.last_match(1).split(/:/).map(&:strip).reject { |x| x =~ /^\$/ }.map { |x| File.join(compilation_dir, x) }
+        else
+          [File.join(compilation_dir, 'modules')]
+        end
+      end
+
       # 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
@@ -34,18 +73,15 @@ def self._convert_file_resources(resources, compilation_dir)
         # that compilation_dir/environments/production is pointing at the right place). Otherwise, try to find
         # compilation_dir/modules. If neither of those exist, this code can't run.
         env_dir = File.join(compilation_dir, 'environments', 'production')
-        unless File.directory?(File.join(env_dir, 'modules'))
-          return unless File.directory?(File.join(compilation_dir, 'modules'))
-          env_dir = compilation_dir
-        end
+        modulepaths = module_path(env_dir) + module_path(compilation_dir)
+        modulepaths.select! { |x| File.directory?(x) }
+        return if modulepaths.empty?
 
-        # Modify the resources
+        # At least one existing module path was found! Run the code to modify the resources.
         resources.map! do |resource|
           if resource_convertible?(resource)
-            # Parse the 'source' parameter into a file on disk
-            src = resource['parameters']['source']
-            raise "Bad parameter source #{src}" unless src =~ %r{^puppet:///modules/([^/]+)/(.+)}
-            path = File.join(env_dir, 'modules', Regexp.last_match(1), 'files', Regexp.last_match(2))
+            path = file_path(resource['parameters']['source'], modulepaths)
+            raise Errno::ENOENT, "Unable to resolve '#{resource['parameters']['source']}'!" if path.nil?
 
             if File.file?(path)
               # If the file is found, read its content. If the content is all ASCII, substitute it into
@@ -60,7 +96,10 @@ def self._convert_file_resources(resources, compilation_dir)
               # However, the fact that we found *something* at this location indicates that the catalog
               # is probably correct. Hence, the very general .exist? check.
             else
-              raise Errno::ENOENT, "Unable to find '#{src}' at #{path}!"
+              # This is probably a bug
+              # :nocov:
+              raise "Unable to find '#{resource['parameters']['source']}' at #{path}!"
+              # :nocov:
             end
           end
           resource

spec/octocatalog-diff/fixtures/catalogs/catalog-modules-test.json

@@ -0,0 +1,58 @@
+{
+  "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/modulestest/manifests/init.pp",
+      "line": 37,
+      "exported": false,
+      "parameters": {
+        "backup": false,
+        "mode": "0440",
+        "owner": "root",
+        "group": "root",
+        "source": "puppet:///modules/modulestest/tmp/modulestest"
+      }
+    },
+    {
+      "type": "File",
+      "title": "/tmp/foobaz",
+      "tags": ["file","class"],
+      "file": "/x/modules/modulestest/manifests/init.pp",
+      "line": 37,
+      "exported": false,
+      "parameters": {
+        "backup": false,
+        "ensure": "directory",
+        "mode": "0755",
+        "owner": "root",
+        "group": "root",
+        "source": "puppet:///modules/modulestest/foo"
+      }
+    }
+  ],
+  "classes": [
+    "test"
+  ]
+}

spec/octocatalog-diff/fixtures/repos/modulepath/environment.conf

@@ -0,0 +1 @@
+modulepath=modules:site:$basemodulepath

spec/octocatalog-diff/fixtures/repos/modulepath/manifests/site.pp

@@ -0,0 +1,4 @@
+node default {
+  include modulestest
+  include sitetest
+}

spec/octocatalog-diff/fixtures/repos/modulepath/modules/modulestest/files/foo/bar.txt

@@ -0,0 +1 @@
+# Hi

spec/octocatalog-diff/fixtures/repos/modulepath/modules/modulestest/files/tmp/modulestest

@@ -0,0 +1 @@
+Modules Test

spec/octocatalog-diff/fixtures/repos/modulepath/modules/modulestest/manifests/init.pp

@@ -0,0 +1,11 @@
+class modulestest {
+  file { '/tmp/modulestest':
+    source => 'puppet:///modules/modulestest/tmp/modulestest',
+  }
+
+  file { '/tmp/foobaz':
+    ensure  => directory,
+    source  => 'puppet:///modules/modulestest/foo',
+    recurse => true,
+  }
+}

spec/octocatalog-diff/fixtures/repos/modulepath/site/sitetest/files/tmp/sitetest

@@ -0,0 +1 @@
+Site Test

spec/octocatalog-diff/fixtures/repos/modulepath/site/sitetest/manifests/init.pp

@@ -0,0 +1,5 @@
+class sitetest {
+  file { '/tmp/sitetest':
+    source => 'puppet:///modules/sitetest/tmp/sitetest',
+  }
+}

spec/octocatalog-diff/integration/convert_file_resources_spec.rb

@@ -118,7 +118,7 @@
       expect(result[:exitcode]).to eq(-1)
       expect(result[:exception]).to be_a_kind_of(OctocatalogDiff::CatalogDiff::Cli::Catalogs::CatalogError)
       expect(result[:exception].message).to match(/failed to compile with Errno::ENOENT/)
-      expect(result[:exception].message).to match(%r{Unable to find 'puppet:///modules/test/foo-new' at})
+      expect(result[:exception].message).to match(%r{Unable to resolve 'puppet:///modules/test/foo-new'})
     end
   end
 end

spec/octocatalog-diff/integration/modulepath_spec.rb

@@ -0,0 +1,131 @@
+require_relative 'integration_helper'
+require OctocatalogDiff::Spec.require_path('/catalog')
+
+describe 'multiple module paths' do
+  # Make sure the catalog compiles correctly, without using any of the file
+  # conversion resources. If the catalog doesn't compile correctly this could
+  # indicate a problem that lies somewhere other than the comparison code.
+  describe 'catalog only' do
+    before(:all) do
+      @result = OctocatalogDiff::Integration.integration(
+        spec_fact_file: 'facts.yaml',
+        spec_repo: 'modulepath',
+        argv: [
+          '--catalog-only',
+          '-n', 'rspec-node.github.net',
+          '--no-compare-file-text'
+        ]
+      )
+      @catalog = OctocatalogDiff::Catalog.new(
+        backend: :json,
+        json: @result[:output]
+      )
+    end
+
+    it 'should compile' do
+      expect(@result[:exitcode]).not_to eq(-1), OctocatalogDiff::Integration.format_exception(@result)
+    end
+
+    it 'should be a valid catalog' do
+      pending 'catalog failed to compile' if @result[:exitcode] == -1
+      expect(@catalog.valid?).to eq(true)
+    end
+
+    it 'should have expected resources in catalog' do
+      pending 'catalog was invalid' unless @catalog.valid?
+      expect(@catalog.resources).to be_a_kind_of(Array)
+
+      mf = @catalog.resource(type: 'File', title: '/tmp/modulestest')
+      expect(mf).to be_a_kind_of(Hash)
+      expect(mf['parameters']).to eq('source' => 'puppet:///modules/modulestest/tmp/modulestest')
+
+      sf = @catalog.resource(type: 'File', title: '/tmp/sitetest')
+      expect(sf).to be_a_kind_of(Hash)
+      expect(sf['parameters']).to eq('source' => 'puppet:///modules/sitetest/tmp/sitetest')
+    end
+  end
+
+  # Test the file comparison feature itself here in its various iterations.
+  describe 'file comparison feature' do
+    before(:each) do
+      @from_dir = Dir.mktmpdir
+      FileUtils.cp_r OctocatalogDiff::Spec.fixture_path('repos/modulepath'), @from_dir
+
+      @to_dir = Dir.mktmpdir
+      FileUtils.cp_r OctocatalogDiff::Spec.fixture_path('repos/modulepath'), @to_dir
+
+      file1 = File.join(@to_dir, 'modulepath', 'modules', 'modulestest', 'files', 'tmp', 'modulestest')
+      File.open(file1, 'w') { |f| f.write("New content of modulestest\n") }
+
+      file2 = File.join(@to_dir, 'modulepath', 'site', 'sitetest', 'files', 'tmp', 'sitetest')
+      File.open(file2, 'w') { |f| f.write("New content of sitetest\n") }
+    end
+
+    after(:each) do
+      OctocatalogDiff::Spec.clean_up_tmpdir(@from_dir)
+      OctocatalogDiff::Spec.clean_up_tmpdir(@to_dir)
+    end
+
+    let(:module_answer) do
+      ['~',
+       "File\f/tmp/modulestest\fparameters\fcontent",
+       "Modules Test\n",
+       "New content of modulestest\n"]
+    end
+
+    let(:site_answer) do
+      [
+        '~',
+        "File\f/tmp/sitetest\fparameters\fcontent",
+        "Site Test\n",
+        "New content of sitetest\n"
+      ]
+    end
+
+    context 'with environment.conf' do
+      # The environment.conf is a fixture within the repository so there is no need
+      # to create it or manipulate it.
+      before(:each) do
+        @result = OctocatalogDiff::Integration.integration(
+          spec_fact_file: 'facts.yaml',
+          argv: [
+            '-n', 'rspec-node.github.net',
+            '--bootstrapped-from-dir', File.join(@from_dir, 'modulepath'),
+            '--bootstrapped-to-dir', File.join(@to_dir, 'modulepath')
+          ]
+        )
+      end
+
+      it 'should compile catalogs and compute differences' do
+        expect(@result[:exitcode]).to eq(2), OctocatalogDiff::Integration.format_exception(@result)
+        expect(@result[:diffs]).to be_a_kind_of(Array)
+        expect(@result[:diffs].size).to eq(2)
+        expect(OctocatalogDiff::Spec.array_contains_partial_array?(@result[:diffs], module_answer)).to eq(true)
+        expect(OctocatalogDiff::Spec.array_contains_partial_array?(@result[:diffs], site_answer)).to eq(true)
+      end
+    end
+
+    context 'without environment.conf in one directory' do
+      before(:each) do
+        FileUtils.rm_f File.join(@from_dir, 'modulepath', 'environment.conf')
+        FileUtils.mv File.join(@from_dir, 'modulepath', 'site', 'sitetest'), File.join(@from_dir, 'modulepath', 'modules')
+        @result = OctocatalogDiff::Integration.integration(
+          spec_fact_file: 'facts.yaml',
+          argv: [
+            '-n', 'rspec-node.github.net',
+            '--bootstrapped-from-dir', File.join(@from_dir, 'modulepath'),
+            '--bootstrapped-to-dir', File.join(@to_dir, 'modulepath')
+          ]
+        )
+      end
+
+      it 'should compile catalogs and compute differences' do
+        expect(@result[:exitcode]).to eq(2), OctocatalogDiff::Integration.format_exception(@result)
+        expect(@result[:diffs]).to be_a_kind_of(Array)
+        expect(@result[:diffs].size).to eq(2)
+        expect(OctocatalogDiff::Spec.array_contains_partial_array?(@result[:diffs], module_answer)).to eq(true)
+        expect(OctocatalogDiff::Spec.array_contains_partial_array?(@result[:diffs], site_answer)).to eq(true)
+      end
+    end
+  end
+end