From cd101f098bd8f9b7a86ebed1a203d6ce3093002a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thore=20B=C3=B6decker?= <me@foxxx0.de>
Date: Wed, 24 Jun 2020 13:10:58 +0200
Subject: [PATCH] print actual template errors on $stderr

Previously the `EvaluatingParser` would implicitly call its own
`#assert_and_report()` method, which in turn hands it over to the
`IssueReporter#assert_and_report()` that tries to utilize puppet's
internal logging but in our rake task this is not properly setup and
thus the logged errors get lost.

The result just leaves the validation summary output, e.g.:
```
Language validation logged 2 errors. Giving up (file: templates/configs/jenkins_casc.sh.epp)
```

Which obviously isn't very helpful to the user.
This commit reworks the epp parsing and validation rake task, so that it
instead prints out a nice summary on `$stderr` that contains all
necessary information, e.g.:
```
ERRORS:
templates/configs/jenkins_casc.sh.epp:1:17: Illegal parameter name. The given name 'CASC_JENKINS_CONFIG' does not conform to the naming rule /^[a-z_]\w*$/
templates/configs/jenkins_casc.sh.epp:4:33: Illegal variable name, The given name 'CASC_JENKINS_CONFIG' does not conform to the naming rule /^((::)?[a-z]\w*)*((::)?[a-z_]\w*)$/
```
---
 lib/puppet-syntax/tasks/puppet-syntax.rb | 13 ++++--
 lib/puppet-syntax/templates.rb           | 56 +++++++++++++++++-------
 spec/puppet-syntax/templates_spec.rb     | 48 +++++++++++---------
 3 files changed, 79 insertions(+), 38 deletions(-)

diff --git a/lib/puppet-syntax/tasks/puppet-syntax.rb b/lib/puppet-syntax/tasks/puppet-syntax.rb
index 8e0c3cf..c100bc3 100644
--- a/lib/puppet-syntax/tasks/puppet-syntax.rb
+++ b/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'
diff --git a/lib/puppet-syntax/templates.rb b/lib/puppet-syntax/templates.rb
index 602bc5b..93db820 100644
--- a/lib/puppet-syntax/templates.rb
+++ b/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
diff --git a/spec/puppet-syntax/templates_spec.rb b/spec/puppet-syntax/templates_spec.rb
index 495a2da..3ee8564 100644
--- a/spec/puppet-syntax/templates_spec.rb
+++ b/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