RFR: 1433: The change of the CSR status doesn't force the CheckWorkItem to re-run the check
Erik Joelsson
erikj at openjdk.java.net
Tue May 31 18:01:36 UTC 2022
On Sat, 28 May 2022 16:47:51 GMT, Guoxiong Li <gli at openjdk.org> wrote:
> Hi all,
>
> Currently, the CheckWorkItem of the PR bot doesn't re-run the check in the following two situations so that the message of the PR body is not updated and then confuses the developers.
>
> Situation 1 steps:
> - The issue has no csr at first
> - Use command `/csr needed` (now the PR has `csr` label)
> - Create a csr for this issue
> - Then the CheckWorkItem doesn't re-run the check (Because the `csr` label is kept in the PR and no new metadata found by CheckWorkItem)
> - (optionally) use command `/csr needed` again
> - (optionally) then the CheckWorkItem also doesn't re-run the check
>
> Situation 2 steps:
> - The issue has no csr at first (now the PR doesn't have `csr` label)
> - Add the new fix version to the existing approved csr. (Now the issue has the csr issue. And because this csr issue had been approved, PR also doesn't have `csr` label.)
> - then the CheckWorkItem doesn't re-run the check (No new metadata found by CheckWorkItem)
> - (optionally) use command `/csr needed`
> - (optionally) then the CheckWorkItem doesn't re-run the check
>
> It is because these two situations don't add or remove the `csr` label so that the `CheckWorkItem#getMetadata` doesn't return new metadata and then `CheckWorkItem#currentCheckValid` returns true and then the check doesn't re-run.
>
> This patch adds a new tag/marker named `<!-- csr:` so that `CheckWorkItem#getMetadata` can generate new metadata from it. When the developers use the `/csr` command or the csr issue status has been modified, the CSRBot or CSRCommand will add the `<!-- csr:` tag to the PR body or the comment. Then the CheckWorkItem will re-run the check because `CheckWorkItem#getMetadata` meets the newly added `<!-- csr:` tag.
>
> Some test cases are added or adjusted.
>
> Thanks for taking the time to review.
>
> Best Regards,
> -- Guoxiong
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.
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) + ");
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.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java line 37:
> 35: private static final String CSR_LABEL = "csr";
> 36: private static final String CSR_NEEDED_MARKER = "<!-- csr: 'needed' -->";
> 37: private static final String CSR_UNNEEDED_MARKER = "<!-- csr: 'unneeded' -->";
Did you intend to look for this in the CSRBot?
-------------
PR: https://git.openjdk.java.net/skara/pull/1327
More information about the skara-dev
mailing list