RFR: 1271: Webrev generation should be optional

[Zhao Song]

On Thu, 16 Feb 2023 19:45:36 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

> 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.

Yesterday, I was also thinking about what's the best way to solve this problem. And I thought my current change would have the least impact to current code. But your opinion is right, it still trigger some unnecessary work. I would think more about it.
And yes, I already made change in `updateWebrevComment`, it won't post comment in pr when webrev is disabled.

> 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.

Sure, will fix it.

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

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