RFR: 2065: Update PR labels when new files are touched [v5]
Erik Joelsson
erikj at openjdk.org
Thu Sep 11 20:46:29 UTC 2025
On Thu, 11 Sep 2025 19:10:24 GMT, Zhao Song <zsong at openjdk.org> wrote:
>> This patch is trying to make the pr bot be able to update PR labels when new files are touched.
>> The main idea is from Erik. For a new PR, the bot will run a labelerWorkItem to auto label the PR first, and the commit hash will be stored in a comment.
>>
>> With this patch, here are some new behaviors of the bot:
>> (1) The user didn't issue manual label command before auto labeling, LabelerWorkItem will do the initial auto labeling and store the commit hash in the comment.
>> (2) If the user issued manual label command before auto labeling, the initial auto labeling will be skipped, the bot would still post a comment and store the hash in the comment.
>> (3) The user pushes a new commit or few commits to a pr that already auto labeled, the bot will evaluate the diff between stored hash and current head, then add new labels or upgrading labels to group labels, in the end, update the stored hash in the comment.
>> (4) The user force pushes to the pr that already auto labeled, the bot will evaluate the diff between baseHash and current head.
>> (5) The user issues a command to add a label to the pr(or even the user add the label via the forge UI), the bot will check if the labels can be upgraded to group labels.
>>
>> The side effect of this feature I can imagine is that a user thinks a file is not related to a component and removed it manually, but later, every time when he touches the file, the label will be added back.
>
> Zhao Song has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 26 additional commits since the last revision:
>
> - update
> - update
> - update
> - update
> - Merge branch 'master' into SKARA-2065
> - fix test
> - fix tests
> - fix tests
> - version2
> - review comment
> - ... and 16 more: https://git.openjdk.org/skara/compare/36a34e12...f704407c
bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelCommand.java line 164:
> 162: reply.println(LabelTracker.addLabelMarker(label));
> 163: reply.println("The `" + groupLabel.get() + "` group label was already applied, so `" + label + "` label will not be added.");
> 164: }
I think you forgot to check that the groupLabel (or rather any of the set of potential group labels) are in `currentLabels`.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelerWorkItem.java line 67:
> 65:
> 66:
> 67: private void updateLabelMessage(List<Comment> comments, List<String> newLabels, String commitHash) {
If this is only supposed to be called once, then the method name should be something like `createInitialLabelMessage`.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelerWorkItem.java line 126:
> 124: @Override
> 125: public Collection<WorkItem> prRun(ScratchArea scratchArea) {
> 126: // If no label configuration, return early
It's probably a good idea to also check if the PR is closed. There could be a delay from scheduling the work item to running it.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelerWorkItem.java line 136:
> 134: Set<String> newLabels = new HashSet<>(pr.labelNames());
> 135:
> 136: // If the initial label comment can be found,Updating labels when new files are touched
Suggestion:
// If the initial label comment can be found, updating labels when new files are touched
bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelerWorkItem.java line 159:
> 157: pr.updateComment(initialLabelComment.get().id(), initialLabelComment.get().body().replaceAll(
> 158: "(<!-- PullRequest Bot label commit ')[^']*(' -->)",
> 159: "$1" + pr.headHash().toString() + "$2"));
Could this be expressed with the existing constants? Look for the pattern and replace with the formatter string?
bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelerWorkItem.java line 170:
> 168: // No need to return CheckWorkItem, if there is any label added, in the next round of CheckWorkItem, it will re-evaluate the pr
> 169: return List.of();
> 170: }
I think it would make the code clearer if the remainder of the method was put in an `else` block here.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelerWorkItem.java line 201:
> 199: .collect(Collectors.joining(", ")));
> 200: message.append(labelsAdded.size() == 1 ? " has" : " have");
> 201: message.append(" been added to this pr based on the files touched in your new commit(s).");
Suggestion:
message.append(" been added to this pull request based on files touched in new commit(s).");
forge/src/main/java/org/openjdk/skara/forge/LabelConfigurationJson.java line 177:
> 175: }
> 176: }
> 177: return Optional.empty();
This method looks like it assumes that a label can only be part of one group. That is true for current uses of label groups, but I don't think the current implementation is making that assumption. I think it would be better if this method returned a collection, probably a set, of group labels instead of an Optional of a single group label. Also please document what the method does.
-------------
PR Review Comment: https://git.openjdk.org/skara/pull/1735#discussion_r2342207086
PR Review Comment: https://git.openjdk.org/skara/pull/1735#discussion_r2342245556
PR Review Comment: https://git.openjdk.org/skara/pull/1735#discussion_r2342267339
PR Review Comment: https://git.openjdk.org/skara/pull/1735#discussion_r2342224657
PR Review Comment: https://git.openjdk.org/skara/pull/1735#discussion_r2342252272
PR Review Comment: https://git.openjdk.org/skara/pull/1735#discussion_r2342256006
PR Review Comment: https://git.openjdk.org/skara/pull/1735#discussion_r2342259059
PR Review Comment: https://git.openjdk.org/skara/pull/1735#discussion_r2342203524
More information about the skara-dev
mailing list