Enhance comment footer on PR messages

[dpordomingo]

fix #472

If the template uses old sprintf format (with %s), lookoutd will log a Warning:

WARN invalid footer template: old footer template with '%s' placeholder is no longer supported:
'_If you have %s feedback about this comment made by the analyzer {{.Name}}, please, [tell us]({{.Feedback}})._'
app=lookoutd

and no footer will be added; same behavior in other cases like: wrong template, no enough data passed to the template...

ussage, from docs in this PR:

Add a Custom Message to the Posted Comments

You can configure source{d} Lookout to add a custom message to every comment that each analyzer returns. This custom message will be created passing to the template defined by providers.github.comment_footer, the name and feedback defined for each analyzer configuration.

If the template (providers.github.comment_footer) is empty, or the analyzer configuration does not define any of the values that the template requires, the custom message won't be added.

Example:

providers:
  github:
    comment_footer: "Comment made by analyzer {{.Name}}, [report]({{.Feedback}})."

Will require that each analyzer defines its name and feedback in order to render its custom message appended to its comments, so:

analyzers:
  - name: Fancy Analyzer
    addr: ipv4://localhost:9930
    feedback: http://example.com/report-issue
  - name: Awesome Analyzer
    addr: ipv4://localhost:9931

Comments from Fancy Analyzer will be appended with the message:

Comment made by analyzer Fancy Analyzer, report.

but comments from Awesome Analyzer wont be appended with a custom message because its configuration lacks of a key defining its feedback value.

[dpordomingo]

ready for a new review plus #495 (comment)

[dpordomingo]

About your .conf.Feedback complain, I'd purpose two different solutions to skip validating whether it is provided or it's not.

• (a) since footer is a go template, we can move the responsibility of avoiding broken links to the source{d} Lookout administrator, in a way that they should provide a reliable template, not looking bad if something is missing
• (b) the problem with .conf.Feedback appears because it has a default value, so template execution returns no error if it is empty; if we move conf.feedback to conf.settings.feedback, and then we execute the template only with the content of conf.Settings, we would benefit from:
  1. no default values for undefined keys
  2. if one field, requested by the template, is not provided (and the template does not use if or with clauses to avoid missing fields), the footer won't be generated,
  3. clear docs: "the footer is built with the value of analyzer .Name and .Settings"
  4. no internal values will be exposed to the template, like conf.Addr or conf.disabled or any other value that could be added to the analyzer config.

option (a) is already implemented by 63fb1fd
if you prefer (b) just let me know.

[dpordomingo]

I'd rather (b), but since it requires a small change in the design, I'd prefer to obtain your validation before start working on it.

[se7entyse7en]

I really like option (b). But it's ok for me to close this as-is and then open another issue for that. 👍

[carlosms]

I prefer a. settings can be overwritten by the end user with .lookout.yml file in their repo. We can reconsider improvements when we introduce the feedback based on comment reactions.

[se7entyse7en]

settings can be overwritten by the end user with .lookout.yml file in their repo.

Oh, right. That's dangerous.

[dpordomingo]

@carlosms @se7entyse7en I see reasonable to let the repo maintainers override the feedback URL that is being used in the comments posted in their repos?

Maybe they misconfigured the analyzer with their local .lookout.yml, and they want to be informed about that. Or maybe that way the repo contributors can ask the repo maintainer to disable the analyzer in that repo because it produces no valuable comments...

[carlosms]

Still, I see that as a different use case
settings was introduced to setup different options for the analyzer, so it works better for the specific repository.

I think it makes sense that the feedback link can be changed by the people deploying lookout and the analyzers (let's say the SaaS admin); but not any users of the SaaS. The comment is posted by the gh app bot, and the contents of those comments should be only managed by the owner of said bot.

What if I installed lookout on my repo, and changed the feedback link to some malicious link? The comment would still be posted as lookout bot, and the admin would be responsible.

[dpordomingo]

So then all PR comments were already addressed.

[carlosms]

👏

[se7entyse7en]

👏