Merge pull request #121 from github/kpaulisse-fact-file-per-branch

Fact file per branch

Author: kpaulisse

lib/octocatalog-diff/cli/options.rb

@@ -112,14 +112,17 @@ def self.option_globally_or_per_branch_string(opts)
           translated = translate_option(opts[:translator], x)
           options[to_option] ||= translated
           options[from_option] ||= translated
+          post_process(opts[:post_process], options)
         end
         parser.on("--to-#{flag}", "#{desc} for the to branch") do |x|
           validate_option(opts[:validator], x) if opts[:validator]
           options[to_option] = translate_option(opts[:translator], x)
+          post_process(opts[:post_process], options)
         end
         parser.on("--from-#{flag}", "#{desc} for the from branch") do |x|
           validate_option(opts[:validator], x) if opts[:validator]
           options[from_option] = translate_option(opts[:translator], x)
+          post_process(opts[:post_process], options)
         end
       end
 
@@ -178,6 +181,15 @@ def self.translate_option(translator, value)
         return value if translator.nil?
         translator.call(value)
       end
+
+      # Code that can run after a translation and operate upon all options. This returns nothing but may
+      # modify options that were input.
+      # @param processor [Code] Processor function
+      # @param options [Hash] Options hash
+      def self.post_process(processor, options)
+        return if processor.nil?
+        processor.call(options)
+      end
     end
   end
 end

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

@@ -7,19 +7,38 @@
   has_weight 150
 
   def parse(parser, options)
-    parser.on('--fact-file FILENAME', 'Fact file to use instead of node lookup') do |fact_file|
-      raise Errno::ENOENT, 'Invalid fact file provided' unless File.file?(fact_file)
-      facts = nil
-      local_opts = { fact_file_string: File.read(fact_file) }
-      if fact_file =~ /\.ya?ml$/
-        facts = OctocatalogDiff::Facts.new(local_opts.merge(backend: :yaml))
-      elsif fact_file =~ /\.json$/
-        facts = OctocatalogDiff::Facts.new(local_opts.merge(backend: :json))
-      else
-        raise ArgumentError, 'I do not know how to parse the provided fact file. Needs .yaml or .json extension.'
+    OctocatalogDiff::Cli::Options.option_globally_or_per_branch(
+      parser: parser,
+      options: options,
+      cli_name: 'fact-file',
+      option_name: 'facts',
+      desc: 'Override fact',
+      datatype: '',
+      validator: ->(fact_file) { File.file?(fact_file) && (fact_file =~ /\.ya?ml$/ || fact_file =~ /\.json$/) },
+      translator: lambda do |fact_file|
+        local_opts = { fact_file_string: File.read(fact_file) }
+        if fact_file =~ /\.ya?ml$/
+          OctocatalogDiff::Facts.new(local_opts.merge(backend: :yaml))
+        elsif fact_file =~ /\.json$/
+          OctocatalogDiff::Facts.new(local_opts.merge(backend: :json))
+        else
+          # :nocov:
+          # Believed to be a bug condition since the validator should kick this out before it ever gets here.
+          raise ArgumentError, 'I do not know how to parse the provided fact file. Needs .yaml or .json extension.'
+          # :nocov:
+        end
+      end,
+      post_process: lambda do |opts|
+        unless options[:node]
+          %w[to_facts from_facts facts].each do |opt|
+            next unless opts[opt.to_sym] && opts[opt.to_sym].node
+            opts[:node] = opts[opt.to_sym].node
+            break
+          end
+        end
+
+        options[:facts] ||= options[:to_facts]
       end
-      options[:facts] = facts
-      options[:node] ||= facts.facts['name']
-    end
+    )
   end
 end

lib/octocatalog-diff/facts.rb

@@ -41,6 +41,15 @@ def dup
       self.class.new(@options, @orig_facts)
     end
 
+    # Node - get the node name, either as set explicitly or as determined from the facts themselves.
+    # @return [String] Node name, explicit or guessed
+    def node
+      return @node unless @node.nil? || @node.empty?
+      return facts['name'] if facts.key?('name')
+      return facts['values']['fqdn'] if facts.key?('values') && facts['values'].key?('fqdn')
+      ''
+    end
+
     # Facts - returned the 'cleansed' facts.
     # Clean up facts by setting 'name' to the node if given, and deleting _timestamp and expiration
     # which may cause Puppet catalog compilation to fail if the facts are old.

spec/octocatalog-diff/fixtures/facts/valid-facts-different-ip.yaml

@@ -0,0 +1,12 @@
+--- !ruby/object:Puppet::Node::Facts
+name: rspec-node.xyz.github.net
+values:
+  _timestamp: '2016-03-16 16:02:13 -0500'
+  apt_update_last_success: 1458162123
+  architecture: amd64
+  clientcert: rspec-node.github.net
+  datacenter: xyz
+  domain: xyz.github.net
+  fqdn: rspec-node.xyz.github.net
+  ipaddress: 10.30.50.70
+  kernel: Linux

spec/octocatalog-diff/integration/fact_file_by_branch_spec.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+require_relative 'integration_helper'
+
+require 'json'
+
+describe 'fact files by branch' 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'),
+        '--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
+
+  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

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

@@ -48,4 +48,30 @@
       end.to raise_error(Errno::ENOENT)
     end
   end
+
+  describe '#opt_to_fact_file' do
+    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')
+      result = run_optparse(['--from-fact-file', fact_file_1, '--to-fact-file', fact_file_2])
+
+      result_facts_1 = result[:from_facts].facts
+      expect(result_facts_1).to be_a_kind_of(Hash)
+      expect(result_facts_1['name']).to eq('rspec-node.xyz.github.net')
+      expect(result_facts_1['values']).to be_a_kind_of(Hash)
+      expect(result_facts_1['values']['fqdn']).to eq('rspec-node.xyz.github.net')
+      expect(result_facts_1['values']['ipaddress']).to be_nil
+      expect(result_facts_1['values'].keys).not_to include('expiration')
+
+      result_facts_2 = result[:to_facts].facts
+      expect(result_facts_2).to be_a_kind_of(Hash)
+      expect(result_facts_2['name']).to eq('rspec-node.xyz.github.net')
+      expect(result_facts_2['values']).to be_a_kind_of(Hash)
+      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')
+
+      expect(result[:facts]).to eq(result[:to_facts])
+    end
+  end
 end

spec/octocatalog-diff/tests/facts_spec.rb

@@ -80,6 +80,32 @@
     end
   end
 
+  describe '#node' do
+    let(:opts) { { backend: :yaml, fact_file_string: File.read(OctocatalogDiff::Spec.fixture_path('facts/facts.yaml')) } }
+
+    it 'should return the node if set explicitly' do
+      obj = OctocatalogDiff::Facts.new(opts.merge(node: 'fluffy-kittens.example.com'))
+      expect(obj.node).to eq('fluffy-kittens.example.com')
+    end
+
+    it 'should return the node from the facts name in the fact file' do
+      obj = OctocatalogDiff::Facts.new(opts)
+      expect(obj.node).to eq('rspec-node.xyz.github.net')
+    end
+
+    it 'should return the FQDN' do
+      obj = OctocatalogDiff::Facts.new(opts)
+      allow(obj).to receive(:facts).and_return('values' => { 'fqdn' => 'foobar.example.com' })
+      expect(obj.node).to eq('foobar.example.com')
+    end
+
+    it 'should return empty if all else fails' do
+      obj = OctocatalogDiff::Facts.new(opts)
+      allow(obj).to receive(:facts).and_return({})
+      expect(obj.node).to eq('')
+    end
+  end
+
   context 'test individual methods' do
     before(:all) do
       opts = {