RFR: 1271: Webrev generation should be optional

[Erik Joelsson]

On Thu, 16 Feb 2023 00:03:32 GMT, Zhao Song <zsong at openjdk.org> wrote:

> Currently, webrevs are generated automatically when a user pushes a new commit, and it's mandatory to choose either JSON or HTML when configuring the mlbridge bot. If both of them are disabled in the configuration, the bot will throw errors.
> 
> In this patch, generating a webrev will be optional. If both JSON and HTML are disabled, no webrev will be generated.

This looks like it solves the stated problem, but the WebrevStorage is still initiated and will still do some work, like creating directories for webrev creation. Ideally I would like that whole part of the bot be optional, but it seems really hard to achieve without having to add null checks all over the place.

I'm not sure what the best solution is here. Maybe we can introduce another `WebrevDescription.Type.NONE` and if neither kind of webrev has been configured, then `WebrevGenerator::generate` would return a `Webrevdescription` of `NONE` type. 

Another possible solution would be to have an `enabled()` method on `WebrevStorage` and wrap all uses of `webrevGenerator` in a check for that. That may get messy though, but would definitely reduce unnecessary work when webrevs are disabled.

Have you looked at `updateWebrevComment`. When webrevs are disabled, we shouldn't post that comment either.

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveMessages.java line 315:

> 313: 
> 314:         return composeDependsOn(pr).map(line -> line + "\n\n").orElse("") +
> 315:                 "Commit messages:\n" +

// When changing this, ensure that the PR pattern in the notifier still matches

Have you verified this?

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveMessages.java line 318:

> 316:                 formatCommitMessagesBrief(commits, commitsLink).orElse("") + "\n\n" +
> 317:                 "Changes: " + pr.changeUrl() + "\n" +
> 318:                 (webrev.uri() == null ? "" : " Webrev: " + webrev.uri().toString()) + "\n" +

I think this will create an empty line instead of the webrev URL. I think it would be better to skip the newline so we just don't print anything.

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveMessages.java line 332:

> 330:         if (webrevs.size() > 0) {
> 331:             if (webrevs.stream().noneMatch(w -> w.uri() != null)) {
> 332:                 webrevLinks = "Webrev is disabled\n\n";

I think this should just be `""` or possibly `"\n"`, not sure.

-------------

PR: https://git.openjdk.org/skara/pull/1475