Add PR triage workflow #17881

mumrah · 2024-11-20T18:04:19Z

Replaces #17298.

This patch adds a simple label based workflow for PRs. PRs opened by non-committers will have a triage label automatically added. This allows committers to see which PRs need to be triaged. After 7 days, a needs-attention label will be added. This serves as a reminder to committers and allows them to see which PRs have languished.

For now, the triage label will need to be manually removed by a committer. In the future, we could try to automate this by removing the label after a PR has been reviewed, assigned, or commented on.

chia7712 · 2024-11-21T01:55:35Z

I totally love this idea!

After 7 days, a needs-attention label will be added. This serves as a reminder to committers and allows them to see which PRs have languished.

Could we just add label "needs-attention" if the PRs from non-committer have no reviewer after 7 days? That may be more simple and it can remind committers to check those languished PRs as well

fvaleri · 2024-11-21T11:26:29Z

I like this, thanks @mumrah.

mumrah · 2024-11-21T17:27:49Z

@chia7712

Could we just add label "needs-attention" if the PRs from non-committer have no reviewer after 7 days?

Unfortunately, pull_request_reviewed has the same issues as pull_request, but there is no pull_request_reviewed_target event to allow editing details of the PR. I tried a few things, but ultimately don't think we can achieve much automation as the result of a review being submitted :(

So, for now it would be a manual process to remove the triage label. If the triage label is removed, the needs-attention label will not be applied.

One improvement we can make is to use https://github.com/actions/stale?tab=readme-ov-file#exempt-all-pr-assignees to prevent needs-attention from being added if the PR has an assignee. This might not be that useful since we rarely use assignee.

Edit: Since we already require adding ci-approved, it seems that requiring a removal or triage is not too burdensome for committers. After all, it is only a few clicks in the UI :)

2nd Edit: IF we ever use GitHub Projects (a big "IF" 😄), then labels like this can be automatically removed when transitioning a PR from one state to another which is a drag-and-drop action in the UI

chia7712 · 2024-11-22T17:53:40Z

.github/workflows/pr-update.yml

+          FOUND_CONTRIBUTOR=$(gh api \
+            -H "Accept: application/vnd.github+json" \
+            -H "X-GitHub-Api-Version: 2022-11-28" \
+            /repos/apache/kafka/contributors --jq '.[] | select(.login == "'"$PR_USER"'") | .login')


does /repos/apache/kafka/contributors return "committers"? it seems to return top contributors

You're right 😖

I think we can query the "orgs" API to check if the author is a member of apache, but that won't tell us if they are a Kafka committer (could be any Apache committer). That would at least prevent committers from having the triage label added.

Another approach is to create a list of committers by GH username. This is related to another improvement I've been contemplating. I'd like to separate the "reviewers" (anyone in community) from the "approvers" (must be committer) in our commit messages. A config file of GH usernames + email + preferred name is how I would approach that one. We could use this config file as the input for this task which would solve the problem.

WDYT?

Another approach is to create a list of committers by GH username.

Maybe we can leverage committers.html?

This is related to another improvement I've been contemplating. I'd like to separate the "reviewers" (anyone in community) from the "approvers" (must be committer) in our commit messages.

I love this idea!

… review

mumrah · 2024-12-02T23:22:33Z

@chia7712 I created a mapping file from the ASF roster and manually matched it up with GitHub user names. https://github.com/mumrah/kafka/blob/asf-committers/README.md

I would like to add this asf-committers as an orphaned branch to this repo so we can use it as a datasource for this PR (and future automations).

WDYT?

chia7712 · 2024-12-03T00:24:02Z

I would like to add this asf-committers as an orphaned branch to this repo so we can use it as a datasource for this PR (and future automations).

My main concern is how to sync the file with committers.html 🤔

mumrah · 2024-12-03T19:22:21Z

I'm not sure it's that much of a problem TBH. There's already a lot of manual steps involved in becoming a committer. I don't think one more PR is too much of a burden :).

Also, the committers.html is more about personal info (photo, links to social, etc). The dataset I'm proposing is more technical to help power GH automations. If the two are out of sync, I can't think of any real negative outcomes. For the workflow in this PR, the worst outcome is that a committer gets a triage label added to their own PRs.

If we wanted to combine the two data sets, I think that would be do-able as well. This would involve moving the photos to the asf-committers branch along with other committer data and then using that to generate the html. Definitely possible.

I'll start a discussion thread about adding the asf-committers branch, since I can't really a PR for it without first creating the orphaned branch.

mimaison · 2024-12-04T09:54:22Z

In https://kafka.apache.org/committers, we have a <div> tag associated with each committer with their GitHub username. For example:

<td>
    David Arthur<br>
    <div class="github_login" hidden>mumrah<br></div>
    Committer, and PMC member<br>
    <a href="[https://www.linkedin.com/in/davidrarthur](view-source:https://www.linkedin.com/in/davidrarthur)">/in/davidarthur</a><br>
    <a href="[https://github.com/mumrah](view-source:https://github.com/mumrah)">github.com/mumrah</a>
</td>

This is what we currently use in https://github.com/apache/kafka/blob/trunk/committer-tools/refresh_collaborators.py

mumrah · 2024-12-04T18:35:12Z

@mimaison good to know! I had no idea we had the GH usernames in a hidden div. Any idea why we don't create a link to the committer's GitHub page? Privacy concerns?

Thanks to @assignUser for the tip on "author_association" 👍

mumrah · 2024-12-04T18:38:34Z

We can see this in action on my fork.

PR opened by me

Testing triage workflow 1 mumrah/kafka#28
Triage label workflow run https://github.com/mumrah/kafka/actions/runs/12166085792/job/33931525395?pr=28
No label is added

PR opened by collaborator

test mumrah/kafka#29
Triage label workflow run https://github.com/mumrah/kafka/actions/runs/12166148750/job/33931734053?pr=29
Triage label is added
Triage label removed after a review

assignUser · 2024-12-04T18:42:55Z

As good practice I'd recommend minimising the token permissions: to just what's need.

mumrah · 2024-12-06T18:26:10Z

@chia7712 or @mimaison any more feedback on this one?

mimaison · 2024-12-10T10:56:09Z

.github/workflows/pr-reviewed-trigger.yml

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+name: Pull Request Reviewed


We're starting to have quite a few actions. I think it would be good to add the description field to all of them to clearly state their role, how they work, and the potential relationships between each others.

I don't think there is a description field for the workflow files. I've been adding comments here and there to help people navigate the workflow files. I think name is as good as we get unfortunately.

Here's the docs: https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#about-yaml-syntax-for-workflows

And a similar question on SO: https://stackoverflow.com/questions/70941956/description-for-a-workflow-in-github-actions

My bad I was looking at https://docs.github.com/en/actions/sharing-automations/creating-actions/metadata-syntax-for-github-actions#description which is not available for workflows.

So let's use comments to describe the workflows. Maybe it's only me (I'm not super familiar with GitHub Actions) but looking at the files (without reading the PR), it's not immediately obvious what each one does.

I just added some docs to the README in the .github/workflows directory. PTAL

Thanks that's very helpful!

mimaison

LGTM

mumrah · 2024-12-10T19:13:47Z

Well, something isn't quite right. I opened a test PR and it was labeled as triage. Looking into it.

mumrah · 2024-12-10T19:32:31Z

Ok, it is working but there is a little quirk. Filed #18128

gharris1727 · 2024-12-10T23:22:25Z

Another quirk: this is causing workflows to fail after reviewing PRs: https://github.com/apache/kafka/actions/runs/12266421343

It doesn't block anything, it just seems to generate nuisance emails.

I submitted this review: #16754 (review)

chia7712 · 2024-12-14T21:46:36Z

.github/workflows/pr-reviewed-trigger.yml

+name: Pull Request Reviewed
+
+on:
+  pull_request_review:


This event brings a interesting race condition:

Approve the Pull Request (PR):

This action triggers the "Pull Request Reviewed" workflow.

Example Run

Add ci-approve:

This action attempts to locate the "pending" run_id.

it mistakenly retrieves the run_id of the "Pull Request Reviewed" workflow, resulting in an error.
Error Example

Perhaps the issue is invalid, as pull requests should not be approved before the CI process completes. 😄

Filed #18200 to (hopefully) fix this

mumrah added 15 commits November 20, 2024 08:31

add triage workflow

c3efb25

print env

003b14e

wip

60df0cd

add committer check

a771e75

fix jq

0350b04

dont exit bash early

fae4258

wip

8961db8

different approach

9fe6121

wip

883f103

wip

35a47bc

hardcode login

cfec4b2

try env without quotes

3211290

try different quoting

22c5b26

add some output

34eecff

formatting

00b96f7

github-actions bot added build Gradle build or GitHub Actions small Small PRs labels Nov 20, 2024

mumrah requested a review from chia7712 November 22, 2024 14:01

chia7712 reviewed Nov 22, 2024

View reviewed changes

Use workflow_run and script action to remove triage label following a…

b954ae0

… review

github-actions bot removed the small Small PRs label Nov 22, 2024

mumrah added 2 commits December 4, 2024 13:07

try different api call

59fd693

Merge remote-tracking branch 'origin/trunk' into triage-workflow-2

3ce7173

mumrah added 3 commits December 4, 2024 13:22

another way

090088a

add owner

935bd12

add comment, clean up if

2833884

mumrah requested a review from chia7712 December 4, 2024 21:56

mimaison reviewed Dec 10, 2024

View reviewed changes

add some GHA docs

ecd5eab

mimaison approved these changes Dec 10, 2024

View reviewed changes

mumrah merged commit b8dadb7 into apache:trunk Dec 10, 2024
8 checks passed

mumrah mentioned this pull request Dec 10, 2024

MINOR add note about org visibility #18128

Merged

chia7712 reviewed Dec 14, 2024

View reviewed changes

mumrah mentioned this pull request Dec 16, 2024

KAFKA-18026: KIP-1112, clean up StatefulProcessorNode #18195

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PR triage workflow #17881

Add PR triage workflow #17881

mumrah commented Nov 20, 2024

chia7712 commented Nov 21, 2024

fvaleri commented Nov 21, 2024

mumrah commented Nov 21, 2024 •

edited

Loading

chia7712 Nov 22, 2024

mumrah Nov 22, 2024 •

edited

Loading

chia7712 Nov 23, 2024

mumrah commented Dec 2, 2024 •

edited

Loading

chia7712 commented Dec 3, 2024

mumrah commented Dec 3, 2024

mimaison commented Dec 4, 2024 •

edited

Loading

mumrah commented Dec 4, 2024

mumrah commented Dec 4, 2024 •

edited

Loading

assignUser commented Dec 4, 2024

mumrah commented Dec 6, 2024 •

edited

Loading

mimaison Dec 10, 2024

mumrah Dec 10, 2024

mimaison Dec 10, 2024

mumrah Dec 10, 2024

mimaison Dec 10, 2024

mimaison left a comment

mumrah commented Dec 10, 2024

mumrah commented Dec 10, 2024

gharris1727 commented Dec 10, 2024 •

edited

Loading

chia7712 Dec 14, 2024

mumrah Dec 15, 2024

Add PR triage workflow #17881

Add PR triage workflow #17881

Conversation

mumrah commented Nov 20, 2024

chia7712 commented Nov 21, 2024

fvaleri commented Nov 21, 2024

mumrah commented Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

mumrah Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumrah commented Dec 2, 2024 • edited Loading

chia7712 commented Dec 3, 2024

mumrah commented Dec 3, 2024

mimaison commented Dec 4, 2024 • edited Loading

mumrah commented Dec 4, 2024

mumrah commented Dec 4, 2024 • edited Loading

assignUser commented Dec 4, 2024

mumrah commented Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimaison left a comment

Choose a reason for hiding this comment

mumrah commented Dec 10, 2024

mumrah commented Dec 10, 2024

gharris1727 commented Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumrah commented Nov 21, 2024 •

edited

Loading

mumrah Nov 22, 2024 •

edited

Loading

mumrah commented Dec 2, 2024 •

edited

Loading

mimaison commented Dec 4, 2024 •

edited

Loading

mumrah commented Dec 4, 2024 •

edited

Loading

mumrah commented Dec 6, 2024 •

edited

Loading

gharris1727 commented Dec 10, 2024 •

edited

Loading