feat: print template line numbers for errors when rendering ERB

[bazzargh]

Feature request

• [ ] If the maintainers agree with the feature as described here, I intend to submit a Pull Request myself.¹ (I would, because I already have the patch described locally, but this is a minor fix and I'd need to wait for our legal dept to clear the CLA)

Proposal: [provide details on the behaviour you'd like to see and why it would be useful]

When there is an error in the values interpolated into the templates, or an error with the partials, the message is confusing to developers because it does not indicate where the error occurred: eg:

[FATAL][2020-03-17 16:47:02 +0530]	Invalid template: deployment.yml.erb
[FATAL][2020-03-17 16:47:02 +0530]	> Error message:
[FATAL][2020-03-17 16:47:02 +0530]	    undefined method `[]' for nil:NilClass

(followed by a 300 line template). This happens for 3 reasons:

1. the stacktrace isn't printed as part of the error;
2. it turns out the stacktraces that are collected are not useful because exceptions are re-thrown without their original context;
3. to understand the line number from the erb, you need to know the line numbering of the merged template, not the numbering from the file on disk.

Point (2): in lib/krane/renderer.rb, there are 3 lines where an error is caught and thrown without its context. In each case a line like:

raise InvalidTemplateError.new(err.message, filename: filename, content: raw_template)

should be replaced with a line like:

raise InvalidTemplateError.new(err.message, filename: filename, content: raw_template), err.message, err.backtrace

Next point (1): in lib/krane/render_task.rb, to print this, we need something like:

debug_msg += "> Stacktrace:\n#{FormattedLogger.indent_four(exception.backtrace.join("\n"))}"

(depending where you print this you will need to add other newlines to debug_msg). This together with the fix for (2) means we get traces like:

[FATAL][2020-03-17 12:24:46 +0000]	> Stacktrace:
[FATAL][2020-03-17 12:24:46 +0000]	    (erb):80:in `template_binding'
[FATAL][2020-03-17 12:24:46 +0000]	    /Users/bewins/.rbenv/versions/2.5.3/lib/ruby/2.5.0/erb.rb:876:in `eval'

Finally to understand what line 80 means in the above, we need to show the numbering of the template. One possible implementation is below:

        numbered = exception.content.lines.map.with_index {|item, index| sprintf("% 4d: %s", index + 1, item)}
        debug_msg += "\n> Template content:\n#{FormattedLogger.indent_four(numbered).join("")}"

¹ _{This is the quickest way to get a new feature! We reserve the right to close feature requests, even ones we like, if the proposer does not intend to contribute to the feature and it doesn't fit in our current roadmap.}

[bazzargh]

actually just spoke to our OSS coordinator and was encouraged to sign the individual CLA. Done. PR incoming

[dturn]

Thanks for taking this on, really excited to see the PR.

I'm not a big fan of printing the stack trace. In the past they've led people to believe krane had crashed. Maybe could you extract the line number and then just print that in a helpful message. We'd also be happy to print less in our errors, as you pointed out 300 lines of templates isn't great. Perhaps we would only print a snipet of the templates around the line with the error?

actually just spoke to our OSS coordinator and was encouraged to sign the individual CLA.

We've noticed most people sign the individual CLA, did your coordinator give a reason why they'd prefer that over the org one? I'm wondering if there is something we can/should pass on to our OSS team.

[bazzargh]

re the CLA: it really is just the black hole of the process. I wanted to open source a ~50 line ruby tool 3 years ago and haven't heard back from legal :D ... so the company decided to delegate authority on things like this (small patches, not releasing our own projects) to permission from manager + individual CLA. We are signed up as an org to other CLAs where they span multiple projects of interest to many of our developers (eg, CNCF)