RFR: 1433: The change of the CSR status doesn't force the CheckWorkItem to re-run the check [v2]

[Guoxiong Li]

On Tue, 31 May 2022 17:44:48 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Only check the Skara generated part. Adjust log. Avoid adding marker multiple times.
>
> bots/csr/src/main/java/org/openjdk/skara/bots/csr/CSRBot.java line 90:
> 
>> 88:                 pr.body().contains(csr.webUrl().toString()) &&
>> 89:                 pr.body().contains(csr.title() + " (**CSR**)");
>> 90:     }
> 
> We could consider making these checks safer by only looking in the Skara generated part of the body. There is a delimiter line you could split on.

Fixed.

> bots/csr/src/main/java/org/openjdk/skara/bots/csr/CSRBot.java line 121:
> 
>> 119:             var csr = csrOptional.get();
>> 120: 
>> 121:             log.info("Found CSR for " + describe(pr) + ". It has id " + csr.id());
> 
> I reacted to this log message the other day. Could you shorten it like this while in the area?
> 
> Suggestion:
> 
>             log.info("Found CSR " + csr.id() + " for " + describe(pr) + ");

Fixed.

> bots/csr/src/main/java/org/openjdk/skara/bots/csr/CSRBot.java line 184:
> 
>> 182:                 // this bot need to add the csr update marker so that the PR bot can update the message of the PR body.
>> 183:                 log.info("CSR closed and approved for " + describe(pr) + ", adding the csr update marker");
>> 184:                 pr.setBody(pr.body() + CSR_UPDATE_MARKER);
> 
> We need to make sure we only add the update marker once. Otherwise we would end up adding a very large amount of these if the PR bot happens to be stuck for a while. I would recommend a method for adding the marker, which first checks if the body contains the marker already.

Fixed.

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

PR: https://git.openjdk.java.net/skara/pull/1327