Merge pull request #8500 from joshcooper/dont_quote_values_10861

(PUP-10861) Don't JSON.dump single fact values

Author: joshcooper

lib/puppet/face/facts.rb

@@ -151,17 +151,23 @@
       options[:resolve_options] = true
       result = Puppet::Node::Facts.indirection.find(Puppet.settings[:certname], options)
 
-      facts = result.values
-
       if options[:value_only]
-        facts.values.first
+        result.values.values.first
       else
-        facts
+        result.values
       end
     end
 
     when_rendering :console do |result|
-      Puppet::Util::Json.dump(result, :pretty => true)
+      # VALID_TYPES = [Integer, Float, TrueClass, FalseClass, NilClass, Symbol, String, Array, Hash].freeze
+      # from https://github.com/puppetlabs/facter/blob/4.0.49/lib/facter/custom_facts/util/normalization.rb#L8
+
+      case result
+      when Array, Hash
+        Puppet::Util::Json.dump(result, :pretty => true)
+      else # one of VALID_TYPES above
+        result
+      end
     end
   end
 end

spec/unit/application/facts_spec.rb

@@ -52,7 +52,12 @@
   end
 
   context 'when show action is called' do
-    let(:expected) { "{\n  \"filesystems\": \"apfs,autofs,devfs\",\n  \"macaddress\": \"64:52:11:22:03:25\"\n}\n" }
+    let(:expected) { <<~END }
+      {
+        "filesystems": "apfs,autofs,devfs",
+        "macaddress": "64:52:11:22:03:25"
+      }
+    END
 
     before :each do
       Puppet::Node::Facts.indirection.terminus_class = :facter
@@ -64,12 +69,58 @@
       expect {
         app.run
       }.to exit_with(0)
-               .and output(expected).to_stdout
+       .and output(expected).to_stdout
+    end
+
+    it 'displays a single fact value' do
+      app.command_line.args << 'filesystems' << '--value-only'
+      expect {
+        app.run
+      }.to exit_with(0)
+       .and output("apfs,autofs,devfs\n").to_stdout
+    end
+
+    it "warns and ignores value-only when multiple fact names are specified" do
+      app.command_line.args << 'filesystems' << 'macaddress' << '--value-only'
+      expect {
+        app.run
+      }.to exit_with(0)
+       .and output(expected).to_stdout
+       .and output(/it can only be used when querying for a single fact/).to_stderr
+    end
+
+    {
+      "type_hash" => [{'a' => 2}, "{\n  \"a\": 2\n}"],
+      "type_array" => [[], "[\n\n]"],
+      "type_string" => ["str", "str"],
+      "type_int" => [1, "1"],
+      "type_float" => [1.0, "1.0"],
+      "type_true" => [true, "true"],
+      "type_false" => [false, "false"],
+      "type_nil" => [nil, ""],
+      "type_sym" => [:sym, "sym"]
+    }.each_pair do |name, values|
+      it "renders '#{name}' as '#{values.last}'" do
+        fact_value = values.first
+        fact_output = values.last
+
+        allow(Facter).to receive(:resolve).and_return({name => fact_value})
+
+        app.command_line.args << name << '--value-only'
+        expect {
+          app.run
+        }.to exit_with(0)
+         .and output("#{fact_output}\n").to_stdout
+      end
     end
   end
 
   context 'when default action is called' do
-    let(:expected) { "---\nfilesystems: apfs,autofs,devfs\nmacaddress: 64:52:11:22:03:25\n" }
+    let(:expected) { <<~END }
+      ---
+      filesystems: apfs,autofs,devfs
+      macaddress: 64:52:11:22:03:25
+    END
 
     before :each do
       Puppet::Node::Facts.indirection.terminus_class = :facter
@@ -81,7 +132,7 @@
       expect {
         app.run
       }.to exit_with(0)
-               .and output(expected).to_stdout
+       .and output(expected).to_stdout
       expect(app.action.name).to eq(:show)
     end
   end