Skip to content

Commit 8d3a3e2

Browse files
bryangingechenPaul-Lez
authored andcommitted
fix: do not remove reactions if the corresponding label is still present on the PR (leanprover-community#27570)
Recently the reaction bot on Zulip has been observed to add a reaction and then remove it shortly afterwards (mainly the reactions corresponding to the `ready-to-merge` and `delegated` labels). The mechanism seems to be the following: - a maintainer writes a comment like "bors merge" (or "bors d+") on a PR that has been `maintainer-merge`'d - `zulip_emoji_merge_delegate.yaml` and `zulip_emoji_merge_delegate_wf.yaml` react to this comment and: - simultaneously adds the "ready-to-merge" and removes the `maintainer-merge` label - adds the "merge" emoji reaction on Zulip - the `zulip_emoji_labelling.yaml` workflow reacts to the `maintainer-merge` label being removed, and clears the "merge" emoji as well. See [this log](https://github.com/leanprover-community/mathlib4/actions/runs/16270158496/job/45935446666#step:5:30) (which removes the "delegated" emoji). We change the `zulip_emoji_labelling.yaml` workflow so that it passes the current list of labels on the PR to the `zulip_emoji_reactions.py` script, and also change the script so that it only removes a reaction if the corresponding label is NOT present. cf. https://leanprover.zulipchat.com/#narrow/channel/345428-mathlib-reviewers/topic/merge.2Fdelegate.20emojis.20bot/near/528289834
1 parent deb78df commit 8d3a3e2

2 files changed

Lines changed: 47 additions & 18 deletions

File tree

.github/workflows/zulip_emoji_labelling.yaml

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ jobs:
4040
PR_NUMBER: ${{ github.event.number}}
4141
LABEL_STATUS: ${{ github.event.action }}
4242
LABEL_NAME: ${{ github.event.label.name }}
43+
PR_LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }}
4344
run: |
44-
printf $'Running the python script with pr "%s"\n' "$PR_NUMBER" "$LABEL_STATUS" "$LABEL"
45-
python scripts/zulip_emoji_reactions.py "$ZULIP_API_KEY" "$ZULIP_EMAIL" "$ZULIP_SITE" "$LABEL_STATUS" "$LABEL_NAME" "$PR_NUMBER"
45+
printf $'Running the python script with:\nPR number: "%s"\nlabel status: "%s"\nlabel: "%s"\nPR labels: "%s"\n' "$PR_NUMBER" "$LABEL_STATUS" "$LABEL" "$PR_LABELS"
46+
python scripts/zulip_emoji_reactions.py "$ZULIP_API_KEY" "$ZULIP_EMAIL" "$ZULIP_SITE" "$LABEL_STATUS" "$LABEL_NAME" "$PR_NUMBER" "$PR_LABELS"

scripts/zulip_emoji_reactions.py

Lines changed: 44 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,16 @@
22
import sys
33
import zulip
44
import re
5+
import json
56

67
# Usage:
7-
# python scripts/zulip_emoji_reactions.py $ZULIP_API_KEY $ZULIP_EMAIL $ZULIP_SITE $ACTION $LABEL_NAME $PR_NUMBER
8+
# python scripts/zulip_emoji_reactions.py $ZULIP_API_KEY $ZULIP_EMAIL $ZULIP_SITE $ACTION $LABEL_NAME $PR_NUMBER $LABELS_TO_KEEP
89
# The first three variables identify the lean4 Zulip chat and allow the bot to access it
910
# (see .github/workflows/zulip_emoji_merge_delegate.yaml),
1011
# see the comment below for a description of $ACTION and $LABEL_NAME.
12+
# $LABELS_TO_KEEP is optional, but if present, should be a JSON array of GitHub PR label names
13+
# Emoji reactions that correspond to these labels will not be removed
14+
# (see .github/workflows/zulip_emoji_labelling.yaml)
1115

1216
ZULIP_API_KEY = sys.argv[1]
1317
ZULIP_EMAIL = sys.argv[2]
@@ -32,6 +36,17 @@
3236
print(f"ACTION: '{ACTION}'")
3337
print(f"LABEL_NAME: '{LABEL_NAME}'")
3438
print(f"PR_NUMBER: '{PR_NUMBER}'")
39+
try:
40+
LABELS_TO_KEEP = sys.argv[7]
41+
print(f"Attempting to parse LABELS_TO_KEEP: '{LABELS_TO_KEEP}")
42+
LABELS_TO_KEEP = json.loads(LABELS_TO_KEEP)
43+
assert isinstance(LABELS_TO_KEEP, list)
44+
assert '' not in LABELS_TO_KEEP
45+
except:
46+
print(f"parsing LABELS_TO_KEEP failed; setting to empty list")
47+
# an empty list is a good default since we remove reactions if the label is `not in LABELS_TO_KEEP`
48+
LABELS_TO_KEEP = []
49+
print(f"LABELS_TO_KEEP: '{LABELS_TO_KEEP}'")
3550

3651
# Initialize Zulip client
3752
client = zulip.Client(
@@ -101,14 +116,22 @@ def has_reaction(name: str) -> bool:
101116
if match:
102117
print(f"matched: '{message}'")
103118

104-
def remove_reaction(name: str, emoji_name: str, **kwargs) -> None:
105-
print(f'Removing {name}')
106-
result = client.remove_reaction({
107-
"message_id": message['id'],
108-
"emoji_name": emoji_name,
109-
**kwargs
110-
})
111-
print(f"result: '{result}'")
119+
# name: a description of the emoji to be removed
120+
# emoji_name: the emoji name as used on Zulip
121+
# label_name: the name of the corresponding PR label, if present in LABELS_TO_KEEP, we do not remove the reaction
122+
# if there is no corresponding label (e.g. for the "closed-pr" reaction) then this should be an empty string ""
123+
# additional arguments will be passed to the zulip library remove_reaction function
124+
def remove_reaction(name: str, emoji_name: str, label_name: str, **kwargs) -> None:
125+
if label_name not in LABELS_TO_KEEP:
126+
print(f'Removing {name}')
127+
result = client.remove_reaction({
128+
"message_id": message['id'],
129+
"emoji_name": emoji_name,
130+
**kwargs
131+
})
132+
print(f"result: '{result}'")
133+
else:
134+
print(f'The "{label_name}" label is present, so we will not remove the "{name}" reaction.')
112135
def add_reaction(name: str, emoji_name: str) -> None:
113136
print(f'adding {name} emoji')
114137
client.add_reaction({
@@ -122,25 +145,30 @@ def add_reaction(name: str, emoji_name: str) -> None:
122145
if ACTION == "labeled":
123146
add_reaction('maintainer-merge', 'hammer')
124147
elif ACTION == "unlabeled":
125-
remove_reaction('maintainer-merge', 'hammer')
148+
remove_reaction('maintainer-merge', 'hammer', 'maintainer-merge')
126149
continue
127150

128151
# We should never remove any "this PR was migrated from a fork" reaction.
129152

130-
# Otherwise, remove all previous mutually exclusive emoji reactions.
153+
# Otherwise, remove all previous mutually exclusive emoji reactions (unless
154+
# LABELS_TO_KEEP contains the corresponding label). This is because otherwise this script
155+
# may end up removing reactions that are still relevant. See PR #27570
156+
# Note that the 'merge' and 'closed-pr' reactions do not have a corresponding label,
157+
# so we pass the empty string (which will never be in LABELS_TO_KEEP) so that they are
158+
# always removed.
131159
# If the emoji is a custom emoji, add the fields `emoji_code` and `reaction_type` as well.
132160
print("Removing previous reactions, if present.")
133161
if has_peace_sign:
134-
remove_reaction('delegated', 'peace_sign')
162+
remove_reaction('delegated', 'peace_sign', 'delegated')
135163
if has_bors:
136-
remove_reaction("bors", "bors", emoji_code="22134", reaction_type="realm_emoji")
164+
remove_reaction("bors", "bors", "ready-to-merge", emoji_code="22134", reaction_type="realm_emoji")
137165
if has_merge:
138-
remove_reaction('merge', 'merge')
166+
remove_reaction('merge', 'merge', "")
139167
if has_awaiting_author:
140-
remove_reaction('awaiting-author', 'writing')
168+
remove_reaction('awaiting-author', 'writing', 'awaiting-author')
141169
if has_closed:
142170
# 61282 was the earlier version of the emoji.
143-
remove_reaction('closed-pr', 'closed-pr', emoji_code="61293", reaction_type="realm_emoji")
171+
remove_reaction('closed-pr', 'closed-pr', '', emoji_code="61293", reaction_type="realm_emoji")
144172

145173
# Apply the appropriate emoji reaction.
146174
print("Applying reactions, as appropriate.")

0 commit comments

Comments
 (0)