print actual template errors on $stderr

lib/puppet-syntax/tasks/puppet-syntax.rb

@@ -48,9 +48,16 @@ def initialize(*args)
           $stderr.puts "---> #{t.name}"
 
           c = PuppetSyntax::Templates.new
-          errors = c.check(filelist_templates)
-          $stdout.puts "#{errors.join("\n")}\n" unless errors.empty?
-          exit 1 unless errors.empty?
+          result = c.check(filelist_templates)
+          unless result[:warnings].empty?
+            $stdout.puts "WARNINGS:"
+            $stdout.puts result[:warnings].join("\n")
+          end
+          unless result[:errors].empty?
+            $stderr.puts "ERRORS:"
+            $stderr.puts result[:errors].join("\n")
+            exit 1
+          end
         end
 
         desc 'Syntax check Hiera config files'

lib/puppet-syntax/templates.rb

@@ -9,38 +9,64 @@ def check(filelist)
 
       # We now have to redirect STDERR in order to capture warnings.
       $stderr = warnings = StringIO.new()
-      errors = []
+      result = { warnings: [], errors: [] }
 
       filelist.each do |file|
         if File.extname(file) == '.epp' or PuppetSyntax.epp_only
-          errors.concat validate_epp(file)
+          tmp = validate_epp(file)
         elsif File.extname(file) == '.erb'
-          errors.concat validate_erb(file)
+          tmp = validate_erb(file)
         end
+        result.merge!(tmp) { |k, a, b| a.concat(b) } unless tmp.nil?
       end
 
       $stderr = STDERR
-      errors << warnings.string unless warnings.string.empty?
-      errors.map! { |e| e.to_s }
+      result[:warnings] << warnings.string unless warnings.string.empty?
 
-      errors
+      result[:errors].map! { |e| e.to_s }
+      result[:warnings].map! { |w| w.to_s }
+
+      result
     end
 
     def validate_epp(filename)
       require 'puppet/pops'
-      errors = []
+      result = { warnings: [], errors: [] }
+      formatter = Puppet::Pops::Validation::DiagnosticFormatterPuppetStyle.new
+      evaluating_parser = Puppet::Pops::Parser::EvaluatingParser::EvaluatingEppParser.new()
+      parser = evaluating_parser.parser()
       begin
-        parser = Puppet::Pops::Parser::EvaluatingParser::EvaluatingEppParser.new()
-        parser.parse_file(filename)
-      rescue => detail
-        errors << detail
+        parse_result = parser.parse_file(filename)
+        validation_result = evaluating_parser.validate(parse_result.model)
+
+        # print out any warnings
+        validation_result.warnings.each do |warn|
+          message = formatter.format_message(warn)
+          file = warn.file
+          line = warn.source_pos.line
+          column = warn.source_pos.pos
+          result[:warnings] << "#{file}:#{line}:#{column}: #{message}"
+        end
+
+        # collect errors and return them in order to indicate validation failure
+        validation_result.errors.each do |err|
+          message = formatter.format_message(err)
+          file = err.file
+          line = err.source_pos.line
+          column = err.source_pos.pos
+          result[:errors] << "#{file}:#{line}:#{column}: #{message}"
+        end
+      rescue Puppet::ParseError, SyntaxError => exc
+        result[:errors] << exc
+      rescue => exc
+        result[:errors] << exc
       end
 
-      errors
+      result
     end
 
     def validate_erb(filename)
-      errors = []
+      result = { warnings: [], errors: [] }
 
       begin
         erb = ERB.new(File.read(filename), nil, '-')
@@ -53,10 +79,10 @@ def validate_erb(filename)
         # This is normal because we don't have the variables that would
         # ordinarily be bound by the parent Puppet manifest.
       rescue SyntaxError => error
-        errors << error
+        result[:errors] << error
       end
 
-      errors
+      result
     end
   end
 end

spec/puppet-syntax/templates_spec.rb

@@ -18,91 +18,99 @@
     files = fixture_templates('pass.erb')
     res = subject.check(files)
 
-    expect(res).to match([])
+    expect(res[:warnings]).to match([])
+    expect(res[:errors]).to match([])
   end
 
   it 'should ignore NameErrors from unbound variables' do
     files = fixture_templates('pass_unbound_var.erb')
     res = subject.check(files)
 
-    expect(res).to match([])
+    expect(res[:warnings]).to match([])
+    expect(res[:errors]).to match([])
   end
 
   it 'should catch SyntaxError' do
     files = fixture_templates('fail_error.erb')
     res = subject.check(files)
 
-    expect(res.size).to eq(1)
-    expect(res[0]).to match(/2: syntax error, unexpected/)
+    expect(res[:errors].size).to eq(1)
+    expect(res[:errors][0]).to match(/2: syntax error, unexpected/)
   end
 
   it 'should catch Ruby warnings' do
     files = fixture_templates('fail_warning.erb')
     res = subject.check(files)
 
-    expect(res.size).to eq(1)
-    expect(res[0]).to match(conditional_warning_regex)
+    expect(res[:warnings].size).to eq(1)
+    expect(res[:warnings][0]).to match(conditional_warning_regex)
   end
 
   it 'should read more than one valid file' do
     files = fixture_templates(['pass.erb', 'pass_unbound_var.erb'])
     res = subject.check(files)
 
-    expect(res).to match([])
+    expect(res[:warnings]).to match([])
+    expect(res[:errors]).to match([])
   end
 
   it 'should continue after finding an error in the first file' do
     files = fixture_templates(['fail_error.erb', 'fail_warning.erb'])
     res = subject.check(files)
 
-    expect(res.size).to eq(2)
-    expect(res[0]).to match(/2: syntax error, unexpected/)
-    expect(res[1]).to match(conditional_warning_regex)
+    expect(res[:warnings].size).to eq(1)
+    expect(res[:errors].size).to eq(1)
+    expect(res[:errors][0]).to match(/2: syntax error, unexpected/)
+    expect(res[:warnings][0]).to match(conditional_warning_regex)
   end
 
   it 'should ignore a TypeError' do
     files = fixture_templates('typeerror_shouldwin.erb')
     res = subject.check(files)
 
-    expect(res).to match([])
+    expect(res[:warnings]).to match([])
+    expect(res[:errors]).to match([])
   end
 
   it 'should ignore files without .erb extension' do
     files = fixture_templates('ignore.tpl')
     res = subject.check(files)
 
-    expect(res).to match([])
+    expect(res[:warnings]).to match([])
+    expect(res[:errors]).to match([])
   end
 
   it 'should return nothing from a valid file' do
     files = fixture_templates('pass.epp')
     res = subject.check(files)
 
-    expect(res).to match([])
+    expect(res[:warnings]).to match([])
+    expect(res[:errors]).to match([])
   end
 
   it 'should catch SyntaxError' do
     files = fixture_templates('fail_error.epp')
     res = subject.check(files)
 
-    expect(res.size).to eq(1)
-    expect(res[0]).to match(/This Type-Name has no effect/)
+    expect(res[:errors].size).to eq(1)
+    expect(res[:errors][0]).to match(/This Type-Name has no effect/)
   end
 
   it 'should read more than one valid file' do
     files = fixture_templates(['pass.epp', 'pass_also.epp'])
     res = subject.check(files)
 
-    expect(res).to match([])
+    expect(res[:warnings]).to match([])
+    expect(res[:errors]).to match([])
   end
 
   it 'should continue after finding an error in the first file' do
     files = fixture_templates(['fail_error.epp', 'fail_error_also.epp'])
     res = subject.check(files)
 
-    expect(res.size).to eq(2)
-    expect(res[0]).to match(/This Type-Name has no effect/)
-    expect(res[1]).to match(/Syntax error at '}' \(file: \S*\/fail_error_also.epp, line: 2, column: 4\)/)
+    expect(res[:errors].size).to eq(2)
+    expect(res[:errors][0]).to match(/This Type-Name has no effect/)
+    expect(res[:errors][1]).to match(/Syntax error at '}' \(file: \S*\/fail_error_also.epp, line: 2, column: 4\)/)
   end
 
   context "when the 'epp_only' options is set" do
@@ -114,7 +122,7 @@
       files = fixture_templates('pass.erb')
       res = subject.check(files)
 
-      expect(res.size).to eq(1)
+      expect(res[:errors].size).to eq(1)
     end
   end
 end