diff --git a/.version b/.version index 7d856835..b49b2533 100644 --- a/.version +++ b/.version @@ -1 +1 @@ -0.5.4 +0.5.6 diff --git a/doc/CHANGELOG.md b/doc/CHANGELOG.md index 80042f09..c738e84d 100644 --- a/doc/CHANGELOG.md +++ b/doc/CHANGELOG.md @@ -8,6 +8,22 @@ +0.5.6 +2016-11-16 + + + + + +0.5.5 +- + +Unreleased internal version + + + 0.5.4 2016-11-07 diff --git a/lib/octocatalog-diff/catalog-util/fileresources.rb b/lib/octocatalog-diff/catalog-util/fileresources.rb index 01112474..a6ba0d0b 100644 --- a/lib/octocatalog-diff/catalog-util/fileresources.rb +++ b/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] 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 diff --git a/spec/octocatalog-diff/fixtures/catalogs/catalog-modules-test.json b/spec/octocatalog-diff/fixtures/catalogs/catalog-modules-test.json new file mode 100644 index 00000000..c37bad9f --- /dev/null +++ b/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" + ] +} diff --git a/spec/octocatalog-diff/fixtures/repos/modulepath/environment.conf b/spec/octocatalog-diff/fixtures/repos/modulepath/environment.conf new file mode 100644 index 00000000..5362de3f --- /dev/null +++ b/spec/octocatalog-diff/fixtures/repos/modulepath/environment.conf @@ -0,0 +1 @@ +modulepath=modules:site:$basemodulepath diff --git a/spec/octocatalog-diff/fixtures/repos/modulepath/manifests/site.pp b/spec/octocatalog-diff/fixtures/repos/modulepath/manifests/site.pp new file mode 100644 index 00000000..3bf4e5bd --- /dev/null +++ b/spec/octocatalog-diff/fixtures/repos/modulepath/manifests/site.pp @@ -0,0 +1,4 @@ +node default { + include modulestest + include sitetest +} diff --git a/spec/octocatalog-diff/fixtures/repos/modulepath/modules/modulestest/files/foo/bar.txt b/spec/octocatalog-diff/fixtures/repos/modulepath/modules/modulestest/files/foo/bar.txt new file mode 100644 index 00000000..76f177f1 --- /dev/null +++ b/spec/octocatalog-diff/fixtures/repos/modulepath/modules/modulestest/files/foo/bar.txt @@ -0,0 +1 @@ +# Hi diff --git a/spec/octocatalog-diff/fixtures/repos/modulepath/modules/modulestest/files/tmp/modulestest b/spec/octocatalog-diff/fixtures/repos/modulepath/modules/modulestest/files/tmp/modulestest new file mode 100644 index 00000000..45ef27ef --- /dev/null +++ b/spec/octocatalog-diff/fixtures/repos/modulepath/modules/modulestest/files/tmp/modulestest @@ -0,0 +1 @@ +Modules Test diff --git a/spec/octocatalog-diff/fixtures/repos/modulepath/modules/modulestest/manifests/init.pp b/spec/octocatalog-diff/fixtures/repos/modulepath/modules/modulestest/manifests/init.pp new file mode 100644 index 00000000..e09fcc49 --- /dev/null +++ b/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, + } +} diff --git a/spec/octocatalog-diff/fixtures/repos/modulepath/site/sitetest/files/tmp/sitetest b/spec/octocatalog-diff/fixtures/repos/modulepath/site/sitetest/files/tmp/sitetest new file mode 100644 index 00000000..7cbe5e7e --- /dev/null +++ b/spec/octocatalog-diff/fixtures/repos/modulepath/site/sitetest/files/tmp/sitetest @@ -0,0 +1 @@ +Site Test diff --git a/spec/octocatalog-diff/fixtures/repos/modulepath/site/sitetest/manifests/init.pp b/spec/octocatalog-diff/fixtures/repos/modulepath/site/sitetest/manifests/init.pp new file mode 100644 index 00000000..5fda225c --- /dev/null +++ b/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', + } +} diff --git a/spec/octocatalog-diff/integration/convert_file_resources_spec.rb b/spec/octocatalog-diff/integration/convert_file_resources_spec.rb index 881b440c..24c8a62f 100644 --- a/spec/octocatalog-diff/integration/convert_file_resources_spec.rb +++ b/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 diff --git a/spec/octocatalog-diff/integration/modulepath_spec.rb b/spec/octocatalog-diff/integration/modulepath_spec.rb new file mode 100644 index 00000000..040756c6 --- /dev/null +++ b/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 diff --git a/spec/octocatalog-diff/tests/catalog-util/fileresources_spec.rb b/spec/octocatalog-diff/tests/catalog-util/fileresources_spec.rb index 58260b59..0e38504c 100644 --- a/spec/octocatalog-diff/tests/catalog-util/fileresources_spec.rb +++ b/spec/octocatalog-diff/tests/catalog-util/fileresources_spec.rb @@ -11,6 +11,89 @@ def catalog_from_fixture(path) OctocatalogDiff::Catalog.new(json: File.read(OctocatalogDiff::Spec.fixture_path(path))) end + describe '#file_path' do + it 'should raise ArgumentError for unexpected format of file name' do + src = 'asldfkjwoeifjslakfj' + expect do + OctocatalogDiff::CatalogUtil::FileResources.file_path(src, []) + end.to raise_error(ArgumentError, /Bad parameter source/) + end + + it 'should return path if file is found' do + allow(File).to receive(:exist?).with('/a/foo/files/bar').and_return(true) + result = OctocatalogDiff::CatalogUtil::FileResources.file_path('puppet:///modules/foo/bar', ['/a']) + expect(result).to eq('/a/foo/files/bar') + end + + it 'should return nil if file is not found' do + allow(File).to receive(:exist?).with('/a/foo/files/bar').and_return(false) + result = OctocatalogDiff::CatalogUtil::FileResources.file_path('puppet:///modules/foo/bar', ['/a']) + expect(result).to eq(nil) + end + end + + describe '#module_path' do + it 'should return "modules" only when environment.conf is missing' do + allow(File).to receive(:file?).with('/a/environment.conf').and_return(false) + result = OctocatalogDiff::CatalogUtil::FileResources.module_path('/a') + expect(result).to eq(['/a/modules']) + end + + it 'should return "modules" if environment.conf has no modulepath' do + allow(File).to receive(:file?).with('/a/environment.conf').and_return(true) + allow(File).to receive(:read).with('/a/environment.conf').and_return('foo') + result = OctocatalogDiff::CatalogUtil::FileResources.module_path('/a') + expect(result).to eq(['/a/modules']) + end + + it 'should return proper entries from environment.conf modulepath' do + allow(File).to receive(:file?).with('/a/environment.conf').and_return(true) + allow(File).to receive(:read).with('/a/environment.conf').and_return('modulepath=modules:site:$basemoduledir') + result = OctocatalogDiff::CatalogUtil::FileResources.module_path('/a') + expect(result).to eq(['/a/modules', '/a/site']) + end + end + + context 'with mixed files and directories' do + describe '#convert_file_resources' do + before(:each) do + @tmpdir = Dir.mktmpdir + FileUtils.cp_r OctocatalogDiff::Spec.fixture_path('repos/modulepath/manifests'), @tmpdir + FileUtils.cp_r OctocatalogDiff::Spec.fixture_path('repos/modulepath/modules'), @tmpdir + Dir.mkdir File.join(@tmpdir, 'environments') + File.symlink @tmpdir, File.join(@tmpdir, 'environments', 'production') + File.open(File.join(@tmpdir, 'manifests', 'site.pp'), 'w') { |f| f.write "include modulestest\n" } + + @obj = catalog_from_fixture('catalogs/catalog-modules-test.json') + @obj.compilation_dir = @tmpdir + @resources_save = @obj.resources.dup + OctocatalogDiff::CatalogUtil::FileResources.convert_file_resources(@obj) + end + + after(:each) do + FileUtils.remove_entry_secure @tmpdir if File.directory?(@tmpdir) + end + + it 'should populate content of a file' do + r = @obj.resources.select { |x| x['type'] == 'File' && x['title'] == '/tmp/foo' } + expect(r).to be_a_kind_of(Array) + expect(r.size).to eq(1) + expect(r.first).to be_a_kind_of(Hash) + expect(r.first['parameters'].key?('source')).to eq(false) + expect(r.first['parameters']['content']).to eq("Modules Test\n") + end + + it 'should leave a directory unmodified' do + r = @obj.resources.select { |x| x['type'] == 'File' && x['title'] == '/tmp/foobaz' } + expect(r).to be_a_kind_of(Array) + expect(r.size).to eq(1) + expect(r.first).to be_a_kind_of(Hash) + expect(r.first['parameters'].key?('content')).to eq(false) + expect(r.first['parameters']['source']).to eq('puppet:///modules/modulestest/foo') + end + end + end + describe '#convert_file_resources' do before(:each) do @tmpdir = Dir.mktmpdir @@ -97,7 +180,7 @@ def catalog_from_fixture(path) # Perform test expect do OctocatalogDiff::CatalogUtil::FileResources.convert_file_resources(obj) - end.to raise_error(Errno::ENOENT, %r{Unable to find 'puppet:///modules/this/does/not/exist'}) + end.to raise_error(Errno::ENOENT, %r{Unable to resolve 'puppet:///modules/this/does/not/exist'}) end it 'should return original if compilation_dir is not a string' do