RFR: 2065: Update PR labels when new files are touched [v5]

[Zhao Song]

On Thu, 11 Sep 2025 20:02:56 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> 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/8bdeb725...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`.

Yes, I realized it and have fixed it.  Thanks

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

Sure, will rename it.

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

Sure, will add it.

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

Makes sense, will change it. Thanks

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

PR Review Comment: https://git.openjdk.org/skara/pull/1735#discussion_r2342296998
PR Review Comment: https://git.openjdk.org/skara/pull/1735#discussion_r2342302866
PR Review Comment: https://git.openjdk.org/skara/pull/1735#discussion_r2342310714
PR Review Comment: https://git.openjdk.org/skara/pull/1735#discussion_r2342298419