From 3a8626fdb17db2a27fd0004418c1aa196d222800 Mon Sep 17 00:00:00 2001
From: Josh Cooper <joshcooper@users.noreply.github.com>
Date: Thu, 28 Jan 2021 12:50:41 -0800
Subject: [PATCH 1/2] (PUP-10861) Use heredoc for readability

Use squiggly heredoc so it's clearer what output we're expecting.
---
 spec/unit/application/facts_spec.rb | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/spec/unit/application/facts_spec.rb b/spec/unit/application/facts_spec.rb
index cb07c908304..a12c037255c 100644
--- a/spec/unit/application/facts_spec.rb
+++ b/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,17 @@
       expect {
         app.run
       }.to exit_with(0)
-               .and output(expected).to_stdout
+       .and output(expected).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 +91,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

From 0de8cfc0e34083e3c3479b0d64099c322044989d Mon Sep 17 00:00:00 2001
From: Josh Cooper <joshcooper@users.noreply.github.com>
Date: Thu, 28 Jan 2021 12:51:51 -0800
Subject: [PATCH 2/2] (PUP-10861) Don't JSON.dump single fact values

The `puppet facts show --value-only` option prints just the value for a single
fact. Previously, the value was pretty printed using JSON.dump, which emitted a
quoted string. Now just print the string, so that the value can be parsed via
automation, like export OSFAMILY=`puppet facts show os.family --value-only`

Also add a test for multiple fact names as the `--value-only` option.
---
 lib/puppet/face/facts.rb            | 16 +++++++----
 spec/unit/application/facts_spec.rb | 41 +++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/lib/puppet/face/facts.rb b/lib/puppet/face/facts.rb
index 57ba05e22ea..9d8805b9751 100644
--- a/lib/puppet/face/facts.rb
+++ b/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
diff --git a/spec/unit/application/facts_spec.rb b/spec/unit/application/facts_spec.rb
index a12c037255c..e23c85afc60 100644
--- a/spec/unit/application/facts_spec.rb
+++ b/spec/unit/application/facts_spec.rb
@@ -71,6 +71,47 @@
       }.to exit_with(0)
        .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