Merge pull request #131 from github/kpaulisse-fact-file-should-not-be-global

Fact file should not be global

Author: kpaulisse

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

@@ -36,8 +36,6 @@ def parse(parser, options)
             break
           end
         end
-
-        options[:facts] ||= options[:to_facts]
       end
     )
   end

spec/octocatalog-diff/integration/fact_file_by_branch_spec.rb

@@ -4,15 +4,15 @@
 
 require 'json'
 
-describe 'fact files by branch' do
+describe 'fact files by branch, with two fact files' do
   before(:all) do
     @result = OctocatalogDiff::Integration.integration_cli(
       [
         '-n', 'rspec-node.github.net',
         '--bootstrapped-to-dir', OctocatalogDiff::Spec.fixture_path('repos/fact-overrides'),
         '--bootstrapped-from-dir', OctocatalogDiff::Spec.fixture_path('repos/fact-overrides'),
         '--output-format', 'json',
-        '--fact-file', OctocatalogDiff::Spec.fixture_path('facts/valid-facts.yaml'),
+        '--from-fact-file', OctocatalogDiff::Spec.fixture_path('facts/valid-facts.yaml'),
         '--to-fact-file', OctocatalogDiff::Spec.fixture_path('facts/valid-facts-different-ip.yaml'),
         '--puppet-binary', OctocatalogDiff::Spec::PUPPET_BINARY,
         '--fact-override', 'foofoo=barbar',
@@ -44,3 +44,101 @@
     expect(@result.stderr).to match(/Diffs computed for rspec-node.github.net/)
   end
 end
+
+describe 'fact files by branch, with only a to fact file' do
+  before(:all) do
+    @tmpdir = Dir.mktmpdir
+    ENV['PUPPET_FACT_DIR'] = @tmpdir
+    FileUtils.cp OctocatalogDiff::Spec.fixture_path('facts/valid-facts.yaml'), File.join(@tmpdir, 'rspec-node.github.net.yaml')
+
+    @result = OctocatalogDiff::Integration.integration_cli(
+      [
+        '-n', 'rspec-node.github.net',
+        '--bootstrapped-to-dir', OctocatalogDiff::Spec.fixture_path('repos/fact-overrides'),
+        '--bootstrapped-from-dir', OctocatalogDiff::Spec.fixture_path('repos/fact-overrides'),
+        '--output-format', 'json',
+        '--to-fact-file', OctocatalogDiff::Spec.fixture_path('facts/valid-facts-different-ip.yaml'),
+        '--puppet-binary', OctocatalogDiff::Spec::PUPPET_BINARY,
+        '--fact-override', 'foofoo=barbar',
+        '-d'
+      ]
+    )
+  end
+
+  after(:all) do
+    ENV['PUPPET_FACT_DIR'] = nil
+    OctocatalogDiff::Spec.clean_up_tmpdir(@tmpdir)
+  end
+
+  it 'should exit with status 2' do
+    expect(@result.exitcode).to eq(2), @result.stderr
+  end
+
+  it 'should contain the correct diffs' do
+    parse_result = JSON.parse(@result.stdout)['diff'].map { |x| OctocatalogDiff::Spec.remove_file_and_line(x) }
+    expect(parse_result.size).to eq(1)
+    expect(parse_result).to include(
+      'diff_type' => '~',
+      'type' => 'File',
+      'title' => '/tmp/ipaddress',
+      'structure' => %w(parameters content),
+      'old_value' => '10.20.30.40',
+      'new_value' => '10.30.50.70'
+    )
+  end
+
+  it 'should log the correct messages' do
+    expect(@result.stderr).to match(/Catalog for . will be built with OctocatalogDiff::Catalog::Computed/)
+    expect(@result.stderr).to match(/Override foofoo from nil to "barbar"/)
+    expect(@result.stderr).to match(/Diffs computed for rspec-node.github.net/)
+  end
+end
+
+describe 'fact files by branch, with only a from fact file' do
+  before(:all) do
+    @tmpdir = Dir.mktmpdir
+    ENV['PUPPET_FACT_DIR'] = @tmpdir
+    FileUtils.cp OctocatalogDiff::Spec.fixture_path('facts/valid-facts.yaml'), File.join(@tmpdir, 'rspec-node.github.net.yaml')
+
+    @result = OctocatalogDiff::Integration.integration_cli(
+      [
+        '-n', 'rspec-node.github.net',
+        '--bootstrapped-to-dir', OctocatalogDiff::Spec.fixture_path('repos/fact-overrides'),
+        '--bootstrapped-from-dir', OctocatalogDiff::Spec.fixture_path('repos/fact-overrides'),
+        '--output-format', 'json',
+        '--from-fact-file', OctocatalogDiff::Spec.fixture_path('facts/valid-facts-different-ip.yaml'),
+        '--puppet-binary', OctocatalogDiff::Spec::PUPPET_BINARY,
+        '--fact-override', 'foofoo=barbar',
+        '-d'
+      ]
+    )
+  end
+
+  after(:all) do
+    ENV['PUPPET_FACT_DIR'] = nil
+    OctocatalogDiff::Spec.clean_up_tmpdir(@tmpdir)
+  end
+
+  it 'should exit with status 2' do
+    expect(@result.exitcode).to eq(2), @result.stderr
+  end
+
+  it 'should contain the correct diffs' do
+    parse_result = JSON.parse(@result.stdout)['diff'].map { |x| OctocatalogDiff::Spec.remove_file_and_line(x) }
+    expect(parse_result.size).to eq(1)
+    expect(parse_result).to include(
+      'diff_type' => '~',
+      'type' => 'File',
+      'title' => '/tmp/ipaddress',
+      'structure' => %w(parameters content),
+      'new_value' => '10.20.30.40',
+      'old_value' => '10.30.50.70'
+    )
+  end
+
+  it 'should log the correct messages' do
+    expect(@result.stderr).to match(/Catalog for . will be built with OctocatalogDiff::Catalog::Computed/)
+    expect(@result.stderr).to match(/Override foofoo from nil to "barbar"/)
+    expect(@result.stderr).to match(/Diffs computed for rspec-node.github.net/)
+  end
+end

spec/octocatalog-diff/tests/cli/options/fact_file_spec.rb

@@ -8,7 +8,7 @@
       fact_file = OctocatalogDiff::Spec.fixture_path('facts/facts.yaml')
       result = run_optparse(['--fact-file', fact_file])
       expect(result[:node]).to eq('rspec-node.xyz.github.net')
-      result_facts = result[:facts].facts
+      result_facts = result[:to_facts].facts
       expect(result_facts).to be_a_kind_of(Hash)
       expect(result_facts['name']).to eq('rspec-node.xyz.github.net')
       expect(result_facts['values']).to be_a_kind_of(Hash)
@@ -20,7 +20,7 @@
       fact_file = OctocatalogDiff::Spec.fixture_path('facts/facts.json')
       result = run_optparse(['--fact-file', fact_file])
       expect(result[:node]).to eq('rspec-node.xyz.github.net')
-      result_facts = result[:facts].facts
+      result_facts = result[:to_facts].facts
       expect(result_facts).to be_a_kind_of(Hash)
       expect(result_facts['name']).to eq('rspec-node.xyz.github.net')
       expect(result_facts['values']).to be_a_kind_of(Hash)
@@ -50,6 +50,20 @@
   end
 
   describe '#opt_to_fact_file' do
+    let(:fact_file) { OctocatalogDiff::Spec.fixture_path('facts/facts.yaml') }
+
+    let(:fact_answer) do
+      {
+        'name' => 'rspec-node.xyz.github.net',
+        'values' => {
+          'apt_update_last_success' => 1_458_162_123,
+          'architecture' => 'amd64',
+          'datacenter' => 'xyz',
+          'fqdn' => 'rspec-node.xyz.github.net'
+        }
+      }
+    end
+
     it 'should distinguish between the to-facts and from-facts' do
       fact_file_1 = OctocatalogDiff::Spec.fixture_path('facts/facts.yaml')
       fact_file_2 = OctocatalogDiff::Spec.fixture_path('facts/valid-facts.yaml')
@@ -70,8 +84,24 @@
       expect(result_facts_2['values']['fqdn']).to eq('rspec-node.xyz.github.net')
       expect(result_facts_2['values']['ipaddress']).to eq('10.20.30.40')
       expect(result_facts_2['values'].keys).not_to include('expiration')
+    end
 
-      expect(result[:facts]).to eq(result[:to_facts])
+    it 'should only define from-fact-file when only --from-fact-file is given' do
+      result = run_optparse(['--from-fact-file', fact_file])
+      expect(result[:from_facts].facts).to eq(fact_answer)
+      expect(result[:to_facts]).to be_nil
+    end
+
+    it 'should only define to-fact-file when only --to-fact-file is given' do
+      result = run_optparse(['--to-fact-file', fact_file])
+      expect(result[:from_facts]).to be_nil
+      expect(result[:to_facts].facts).to eq(fact_answer)
+    end
+
+    it 'should define both from and to fact file when --fact-file is given' do
+      result = run_optparse(['--fact-file', fact_file])
+      expect(result[:from_facts].facts).to eq(fact_answer)
+      expect(result[:to_facts].facts).to eq(fact_answer)
     end
   end
 end