Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

print actual template errors on $stderr #125

Merged
merged 1 commit into from
Jun 24, 2020

Conversation

foxxx0
Copy link
Member

@foxxx0 foxxx0 commented Jun 24, 2020

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*)$/

@foxxx0 foxxx0 added the enhancement New feature or request label Jun 24, 2020
@foxxx0 foxxx0 requested a review from bastelfreak June 24, 2020 11:19
@foxxx0 foxxx0 self-assigned this Jun 24, 2020
Copy link

@hlindberg hlindberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me - it is perfectly ok to use the parser inside the EvaluatingParser as it is considered API. You may want to also add output of any warnings or users may miss deprecations etc.

@foxxx0 foxxx0 force-pushed the show-template-errors branch from 4bb029f to 0c6ba2d Compare June 24, 2020 11:33
@foxxx0
Copy link
Member Author

foxxx0 commented Jun 24, 2020

great point, added STDOUT.puts() for the warnings as well.

also parser exceptions are now properly rescued and added to the errors[] array, which make rake spec pass again.

@foxxx0 foxxx0 force-pushed the show-template-errors branch from 0c6ba2d to b401f48 Compare June 24, 2020 11:37
@foxxx0 foxxx0 force-pushed the show-template-errors branch from b401f48 to a3e52af Compare June 24, 2020 12:28
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*)$/
```
@foxxx0 foxxx0 force-pushed the show-template-errors branch from a3e52af to cd101f0 Compare June 24, 2020 12:33
@foxxx0 foxxx0 merged commit 9e01301 into voxpupuli:master Jun 24, 2020
@foxxx0 foxxx0 deleted the show-template-errors branch June 24, 2020 13:14
@foxxx0 foxxx0 mentioned this pull request Jun 24, 2020
@hlindberg
Copy link

hlindberg commented Jun 24, 2020

Good work on this! I think there is one more case to consider wrt. exceptions - I think there may be a LexError as well if the template does not even lex ok - for example a malformed number, string that isn't closed etc.

I am not sure if this case is covered by the already rescued exceptions.

@foxxx0
Copy link
Member Author

foxxx0 commented Jun 24, 2020

There is a generic "catchall" rescue here: https://github.com/voxpupuli/puppet-syntax/pull/125/files#diff-a8986ba30354c68e43b0dc3441e884a2R61 which should take care of that.

I was a bit torn between listing all of the possible expected Exceptions and just rescueing everything, so now it's a bit of both but no Exception should get lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants