(PUP-11725) Turn on strict mode by default

AriaXLi · AriaXLi · commit 65bcabdd50df · 2023-02-07T14:40:57.000-08:00
To start moving towards enabling strict mode by default, in PUP-11689 strict_variables was turned on. This commit continues that by setting strict to default to error instead of warning. Additionally, rspec tests that failed after the change were updated. I came across some unexpected behavior after setting strict to error so I set those tests to pending and created a ticket, PUP-11751, so those could be looked into later.
diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb
@@ -188,7 +188,7 @@ def self.initialize_default_settings!(settings)
         ",
     },
     :strict => {
-      :default    => :warning,
+      :default    => :error,
       :type       => :symbolic_enum,
       :values     => [:off, :warning, :error],
       :desc       => "The strictness level of puppet. Allowed values are:
diff --git a/spec/integration/application/apply_spec.rb b/spec/integration/application/apply_spec.rb
@@ -203,6 +203,7 @@ class mod {
     end
 
     it 'can apply the catalog' do
+      Puppet[:strict] = :warning
       catalog = compile_to_catalog('include mod', node)
 
       Puppet[:environment] = env_name
@@ -212,6 +213,20 @@ class mod {
         apply.run
       }.to output(%r{Notify\[The Street 23\]/message: defined 'message' as 'The Street 23'}).to_stdout
     end
+
+    it 'can apply the catalog with no warning' do
+      pending("See PUP-11751")
+      Puppet[:strict] = :warning
+      logs = []
+      Puppet::Util::Log.with_destination(Puppet::Test::LogCollector.new(logs)) do
+        catalog = compile_to_catalog('include mod', node)
+        Puppet[:environment] = env_name
+        apply.command_line.args = ['--catalog', file_containing('manifest', catalog.to_json)]
+        apply.run
+      end
+      # expected to have no warnings
+      expect(logs.select { |log| log.level == :warning }.map { |log| log.message }).to be_empty
+    end
   end
 
   it "raises if the environment directory does not exist" do
@@ -438,6 +453,11 @@ def bogus()
     end
 
     context 'and the file is not serialized with rich_data' do
+      # do not want to stub out behavior in tests
+      before :each do 
+        Puppet[:strict] = :warning
+      end
+
       it 'will notify a string that is the result of Regexp#inspect (from Runtime3xConverter)' do
         catalog = compile_to_catalog(execute, node)
         apply.command_line.args = ['--catalog', file_containing('manifest', catalog.to_json)]
diff --git a/spec/integration/parser/compiler_spec.rb b/spec/integration/parser/compiler_spec.rb
@@ -158,7 +158,7 @@ class c inherits b { notify { hi: } }
     it 'makes $settings::strict available as string' do
       node = Puppet::Node.new("testing")
       catalog = compile_to_catalog(<<-MANIFEST, node)
-          notify { 'test': message => $settings::strict == 'warning' }
+          notify { 'test': message => $settings::strict == 'error' }
       MANIFEST
       expect(catalog).to have_resource("Notify[test]").with_parameter(:message, true)
     end
@@ -174,7 +174,7 @@ class c inherits b { notify { hi: } }
     it 'makes all server settings available as $settings::all_local hash' do
       node = Puppet::Node.new("testing")
       catalog = compile_to_catalog(<<-MANIFEST, node)
-          notify { 'test': message => $settings::all_local['strict'] == 'warning' }
+          notify { 'test': message => $settings::all_local['strict'] == 'error' }
       MANIFEST
       expect(catalog).to have_resource("Notify[test]").with_parameter(:message, true)
     end
@@ -694,7 +694,7 @@ class a { $_a = 10}
       end
 
       it 'a missing variable as default causes an evaluation error' do
-        # when strict variables not on
+        # strict mode on by default for 8.x
         expect {
           compile_to_catalog(<<-MANIFEST)
           class a ($b=$x) { notify {test: message=>"yes ${undef == $b}" } }
@@ -703,9 +703,9 @@ class a ($b=$x) { notify {test: message=>"yes ${undef == $b}" } }
         }.to raise_error(/Evaluation Error: Unknown variable: 'x'/)
       end
 
-      it 'a missing variable as default value becomes undef when strict_variables is off' do
-        # strict variables on by default for 8.x
+      it 'a missing variable as default value becomes undef when strict mode is off' do
         Puppet[:strict_variables] = false
+        Puppet[:strict] = :warning
         catalog = compile_to_catalog(<<-MANIFEST)
         class a ($b=$x) { notify {test: message=>"yes ${undef == $b}" } }
           include a
diff --git a/spec/integration/parser/conditionals_spec.rb b/spec/integration/parser/conditionals_spec.rb
@@ -68,8 +68,9 @@
     end
 
     it "evaluates undefined variables as false" do
-      # strict_variables is off so behavior this test is trying to check isn't stubbed out
+      # strict mode is off so behavior this test is trying to check isn't stubbed out
       Puppet[:strict_variables] = false
+      Puppet[:strict] = :warning
       catalog = compile_to_catalog(<<-CODE)
       if $undef_var {
       } else {
diff --git a/spec/integration/parser/scope_spec.rb b/spec/integration/parser/scope_spec.rb
@@ -276,8 +276,9 @@ class a inherits parent { }
 
     ['a:.b', '::a::b'].each do |ref|
       it "does not resolve a qualified name on the form #{ref} against top scope" do
-        # strict_variables is off so behavior this test is trying to check isn't stubbed out
+        # strict mode is off so behavior this test is trying to check isn't stubbed out
         Puppet[:strict_variables] = false
+        Puppet[:strict] = :warning
         expect_the_message_not_to_be("from topscope") do <<-"MANIFEST"
               class c {
                 notify { 'something': message => "$#{ref}" }
@@ -299,8 +300,9 @@ class a inherits parent { }
 
     ['a:.b', '::a::b'].each do |ref|
       it "does not resolve a qualified name on the form #{ref} against node scope" do
-        # strict_variables is off so behavior this test is trying to check isn't stubbed out
+        # strict mode is off so behavior this test is trying to check isn't stubbed out
         Puppet[:strict_variables] = false
+        Puppet[:strict] = :warning
         expect_the_message_not_to_be("from node") do <<-MANIFEST
               class c {
                 notify { 'something': message => "$a::b" }
diff --git a/spec/unit/application/lookup_spec.rb b/spec/unit/application/lookup_spec.rb
@@ -503,6 +503,9 @@ def run_lookup(lookup)
       include PuppetSpec::Files
 
       it "is unaffected by global variables unless '--compile' is used" do
+        # strict mode is off so behavior this test is trying to check isn't stubbed out
+        Puppet[:strict_variables] = false
+        Puppet[:strict] = :warning
         lookup.options[:node] = node
         lookup.options[:render_as] = :s
         allow(lookup.command_line).to receive(:args).and_return(['c'])
@@ -673,8 +676,9 @@ def run_lookup(lookup)
       let(:node) { Puppet::Node.new("testnode", :facts => facts, :environment => 'puppet_func_provider') }
 
       it "works OK in the absense of '--compile'" do
-        # strict_variables is off so behavior this test is trying to check isn't stubbed out
+        # strict mode is off so behavior this test is trying to check isn't stubbed out
         Puppet[:strict_variables] = false
+        Puppet[:strict] = :warning
         lookup.options[:node] = node
         allow(lookup.command_line).to receive(:args).and_return(['c'])
         lookup.options[:render_as] = :s
diff --git a/spec/unit/application/resource_spec.rb b/spec/unit/application/resource_spec.rb
@@ -129,22 +129,24 @@
   end
 
   describe "when printing output" do
-    it "should not emit puppet class tags when printing yaml" do
-      Puppet::Type.newtype(:stringify) do
-        ensurable
-        newparam(:name, isnamevar: true)
-        newproperty(:string)
-      end
+    Puppet::Type.newtype(:stringify) do
+      ensurable
+      newparam(:name, isnamevar: true)
+      newproperty(:string)
+    end
 
-      Puppet::Type.type(:stringify).provide(:stringify) do
-        def exists?
-          true
-        end
+    Puppet::Type.type(:stringify).provide(:stringify) do
+      def exists?
+        true
+      end
 
-        def string
-          Puppet::Util::Execution::ProcessOutput.new('test', 0)
-        end
+      def string
+        Puppet::Util::Execution::ProcessOutput.new('test', 0)
       end
+    end
+    
+    it "should not emit puppet class tags when printing yaml when strict mode is off" do
+      Puppet[:strict] = :warning
 
       @resource_app.options[:to_yaml] = true
       allow(@resource_app.command_line).to receive(:args).and_return(['stringify', 'hello', 'ensure=present', 'string=asd'])
@@ -158,6 +160,13 @@ def string
       expect { @resource_app.main }.not_to raise_error
     end
 
+    it "should raise an error when printing yaml by default" do
+      @resource_app.options[:to_yaml] = true
+      allow(@resource_app.command_line).to receive(:args).and_return(['stringify', 'hello', 'ensure=present', 'string=asd'])
+      expect { @resource_app.main }.to raise_error( Puppet::PreformattedError,
+        /Stringify\[hello\]\['string'\] contains a Puppet::Util::Execution::ProcessOutput value. It will be converted to the String 'test'/)
+    end
+
     it "should ensure all values to be printed are in the external encoding" do
       resources = [
         Puppet::Type.type(:user).new(:name => "\u2603".force_encoding(Encoding::UTF_8)).to_resource,
diff --git a/spec/unit/data_providers/hiera_data_provider_spec.rb b/spec/unit/data_providers/hiera_data_provider_spec.rb
@@ -35,6 +35,7 @@ def compile(environment, code = nil)
     node = Puppet::Node.new("testnode", :facts => facts, :environment => environment)
     compiler = Puppet::Parser::Compiler.new(node)
     compiler.topscope['domain'] = 'example.com'
+    compiler.topscope['my_fact'] = 'server3'
     block_given? ? compiler.compile { |catalog| yield(compiler); catalog } : compiler.compile
   end
 
@@ -164,6 +165,7 @@ def extract_notifications(catalog)
     expect(extract_notifications(compiler.compile)).to include('server2')
 
     compiler = Puppet::Parser::Compiler.new(node)
+    compiler.topscope['my_fact'] = 'server3'
     expect(extract_notifications(compiler.compile)).to include('In name.yaml')
   end
 
diff --git a/spec/unit/functions/epp_spec.rb b/spec/unit/functions/epp_spec.rb
@@ -22,8 +22,9 @@
       expect { eval_template("<%= $kryptonite == undef %>")}.to raise_error(/Evaluation Error: Unknown variable: 'kryptonite'./)
     end
 
-    it "get nil accessing a variable that does not exist when strict_variables is off" do
+    it "get nil accessing a variable that does not exist when strict mode is off" do
       Puppet[:strict_variables] = false
+      Puppet[:strict] = :warning
       expect(eval_template("<%= $kryptonite == undef %>")).to eq("true")
     end
 
diff --git a/spec/unit/functions/inline_epp_spec.rb b/spec/unit/functions/inline_epp_spec.rb
@@ -20,8 +20,9 @@
       expect { eval_template("<%= $kryptonite == undef %>")}.to raise_error(/Evaluation Error: Unknown variable: 'kryptonite'./)
     end
 
-    it "get nil accessing a variable that does not exist when strict_variables is off" do
+    it "get nil accessing a variable that does not exist when strict mode is off" do
       Puppet[:strict_variables] = false
+      Puppet[:strict] = :warning
       expect(eval_template("<%= $kryptonite == undef %>")).to eq("true")
     end
 
diff --git a/spec/unit/functions/lookup_spec.rb b/spec/unit/functions/lookup_spec.rb
@@ -573,9 +573,11 @@ def explain(key, options = {})
         it 'reloads the configuration if interpolated values change' do
           Puppet[:log_level] = 'debug'
           collect_notices("notice('success')") do |scope|
+            scope['var'] = {}
             expect(lookup_func.call(scope, 'y')).to eql('value y from x')
-            scope['var'] = { 'sub' => '_d' }
-            expect(lookup_func.call(scope, 'y')).to eql('value y from x_d')
+            nested_scope = scope.compiler.newscope(scope)
+            nested_scope['var'] = { 'sub' => '_d' }
+            expect(lookup_func.call(nested_scope, 'y')).to eql('value y from x_d')
             nested_scope = scope.compiler.newscope(scope)
             nested_scope['var'] = { 'sub' => '_e' }
             expect(lookup_func.call(nested_scope, 'y')).to eql('value y from x_e')
@@ -602,15 +604,10 @@ def explain(key, options = {})
       context 'using global variable reference' do
         let(:data_path) { 'x%{::var.sub}.yaml' }
 
-        it 'reloads the configuration if interpolated that was previously undefined, gets defined' do
-          Puppet[:log_level] = 'debug'
+        it 'raises an error when reloads the configuration if interpolating undefined values' do
           collect_notices("notice('success')") do |scope|
-            expect(lookup_func.call(scope, 'y')).to eql('value y from x')
-            scope['var'] = { 'sub' => '_d' }
-            expect(lookup_func.call(scope, 'y')).to eql('value y from x_d')
+            expect { lookup_func.call(scope, 'y') }.to raise_error(/Undefined variable '::var'/)
           end
-          expect(notices).to eql(['success'])
-          expect(debugs.any? { |m| m =~ /Hiera configuration recreated due to change of scope variables used in interpolation expressions/ }).to be_truthy
         end
 
         it 'does not reload the configuration if value changes locally' do
@@ -726,10 +723,13 @@ def explain(key, options = {})
         it 'interpolates both key and value"' do
           Puppet[:log_level] = 'debug'
           collect_notices("notice('success')") do |scope|
+            scope['key'] = ''
+            scope['value'] = ''
             expect(lookup_func.call(scope, 'a')).to eql({'' => 'the '})
-            scope['key'] = 'a_key'
-            scope['value'] = 'interpolated value'
-            expect(lookup_func.call(scope, 'a')).to eql({'a_key' => 'the interpolated value'})
+            nested_scope = scope.compiler.newscope(scope)
+            nested_scope['key'] = 'a_key'
+            nested_scope['value'] = 'interpolated value'
+            expect(lookup_func.call(nested_scope, 'a')).to eql({'a_key' => 'the interpolated value'})
           end
           expect(notices).to eql(['success'])
         end
@@ -783,15 +783,26 @@ def explain(key, options = {})
       context 'that contains a legal yaml that is not a hash' do
         let(:common_yaml) { "- A list\n- of things" }
 
-        it 'fails with a "did not find"' do
+        it 'fails with a "invalid yaml hash"' do
           expect { lookup('a') }.to raise_error do |e|
-            expect(e.message).to match(/did not find a value for the name 'a'/)
+            expect(e.message).to match(/spec\/data\/common.yaml: file does not contain a valid yaml hash/)
           end
         end
 
-        it 'logs a warning that the file does not contain a hash' do
-          expect { lookup('a') }.to raise_error(Puppet::DataBinding::LookupError)
-          expect(warnings).to include(/spec\/data\/common.yaml: file does not contain a valid yaml hash/)
+        context 'when strict mode is off' do
+          before :each do
+            Puppet[:strict] = :warning
+          end
+          it 'fails with a "did not find"' do
+            expect { lookup('a') }.to raise_error do |e|
+              expect(e.message).to match(/did not find a value for the name 'a'/)
+            end
+          end
+
+          it 'logs a warning that the file does not contain a hash' do
+            expect { lookup('a') }.to raise_error(Puppet::DataBinding::LookupError)
+            expect(warnings).to include(/spec\/data\/common.yaml: file does not contain a valid yaml hash/)
+          end
         end
       end
 
@@ -2412,6 +2423,7 @@ def uri_test_func(options, context)
         end
 
         it 'defaults are used when data is not found in scope interpolations' do
+          pending('See PUP-11751')
           expect(lookup('mod_a::interpolate_scope_xd', { 'default_values_hash' => defaults })).to eql('-- value scope_xd (from default) --')
         end
 
@@ -2449,15 +2461,16 @@ def uri_test_func(options, context)
           expect(lookup('mod_a::interpolate_scope')).to eql('-- scope scalar value --')
         end
 
-        it 'interpolates not found in scope as empty string' do
-          expect(lookup('mod_a::interpolate_scope_not_found')).to eql('--  --')
+        it 'raises an error when trying to interpolate not found in scope' do
+          expect { lookup('mod_a::interpolate_scope_not_found') 
+          }.to raise_error(/Evaluation Error: Error while evaluating a Function Call, Undefined variable 'scope_nope';/)
         end
 
         it 'interpolates dotted key from scope' do
           expect(lookup('mod_a::interpolate_scope_dig')).to eql('-- scope hash a --')
         end
 
-        it 'treates interpolated dotted key but not found in scope as empty string' do
+        it 'treats interpolated dotted key but not found in scope as empty string' do
           expect(lookup('mod_a::interpolate_scope_dig_not_found')).to eql('--  --')
         end
 
diff --git a/spec/unit/functions/return_spec.rb b/spec/unit/functions/return_spec.rb
@@ -19,8 +19,10 @@
     end
 
     it 'with undef value as function result when not given an argument' do
-      # strict_variables is off so behavior this test is trying to check isn't stubbed out
+      # strict mode is off so behavior this test is trying to check isn't stubbed out
       Puppet[:strict_variables] = false
+      Puppet[:strict] = :warning
+
       expect(compile_to_catalog(<<-CODE)).to have_resource('Notify[xy]')
           function please_return() {
             [1,2,3].map |$x| { if $x == 1 { return() } 200 }
diff --git a/spec/unit/hiera/scope_spec.rb b/spec/unit/hiera/scope_spec.rb
@@ -16,10 +16,15 @@
   end
 
   describe "#[]" do
-    it "should return nil when no value is found" do
+    it "should return nil when no value is found and strict mode is off" do
+      Puppet[:strict] = :warning
       expect(scope["foo"]).to eq(nil)
     end
 
+    it "should raise error by default when no value is found" do
+      expect { scope["foo"] }.to raise_error(/Undefined variable 'foo'/)
+    end
+
     it "should treat '' as nil" do
       real["foo"] = ""
 
diff --git a/spec/unit/module_spec.rb b/spec/unit/module_spec.rb
@@ -773,12 +773,14 @@
   end
 
   it "should not have metadata if it has a metadata file and its data is empty" do
+    Puppet[:strict] = :warning
     allow(File).to receive(:read).with(mymod_metadata, {:encoding => 'utf-8'}).and_return("")
 
     expect(mymod).not_to be_has_metadata
   end
 
   it "should not have metadata if has a metadata file and its data is invalid" do
+    Puppet[:strict] = :warning
     allow(File).to receive(:read).with(mymod_metadata, {:encoding => 'utf-8'}).and_return("This is some invalid json.\n")
     expect(mymod).not_to be_has_metadata
   end
@@ -799,12 +801,6 @@
     Puppet::Module.new("yay", "/path", double("env"))
   end
 
-  it "should tolerate failure to parse" do
-    allow(File).to receive(:read).with(mymod_metadata, {:encoding => 'utf-8'}).and_return(my_fixture('trailing-comma.json'))
-
-    expect(mymod.has_metadata?).to be_falsey
-  end
-
   describe 'when --strict is warning' do
     before :each do
       Puppet[:strict] = :warning
diff --git a/spec/unit/parser/scope_spec.rb b/spec/unit/parser/scope_spec.rb
diff --git a/spec/unit/pops/evaluator/arithmetic_ops_spec.rb b/spec/unit/pops/evaluator/arithmetic_ops_spec.rb
diff --git a/spec/unit/pops/evaluator/evaluating_parser_spec.rb b/spec/unit/pops/evaluator/evaluating_parser_spec.rb
diff --git a/spec/unit/pops/loaders/loaders_spec.rb b/spec/unit/pops/loaders/loaders_spec.rb
diff --git a/spec/unit/pops/serialization/to_from_hr_spec.rb b/spec/unit/pops/serialization/to_from_hr_spec.rb
diff --git a/spec/unit/pops/validator/validator_spec.rb b/spec/unit/pops/validator/validator_spec.rb
diff --git a/spec/unit/resource/catalog_spec.rb b/spec/unit/resource/catalog_spec.rb
