Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bot commands in new issues sensitive to , after bot user name #530

Open
1 task done
kbrindley opened this issue Jan 30, 2023 · 15 comments
Open
1 task done

Bot commands in new issues sensitive to , after bot user name #530

kbrindley opened this issue Jan 30, 2023 · 15 comments
Labels

Comments

@kbrindley
Copy link

kbrindley commented Jan 30, 2023

Solution to issue cannot be found in the documentation.

  • I checked the documentation.

Issue

https://conda-forge.org/docs/maintainer/updating_pkgs.html#updating-the-maintainer-list

The "updating the maintainer list" documentation describes adding an issue with the title

@conda-forge-admin, please add user @username

to trigger an automated process that updates the maintainer list. However, this title doesn't trigger the automated process. The workaround is to delete the comma as

@conda-forge-admin please add user @username

which then does trigger the automated process.

Installed packages

Not applicable

Environment info

Not applicable
@kbrindley
Copy link
Author

Existing tests appear to catch the existing documentation example

@conda-forge-admin, please add user @username

https://github.com/conda-forge/conda-forge-webservices/blob/main/conda_forge_webservices/tests/test_commands.py#L271

'@conda-forge-admin, please add user @blah',

Possibly related to the user name in the linked behavior example, which had mixed case and a hyphen? Perhaps the user is captured correctly when the comma is excluded?

@kbrindley
Copy link
Author

I can't run the tests locally. Poking around the commands.py source, I think I can re-create the RegEx. I don't think my observed behavior is a RegEx issue. This is probably about as far as I can chase this issue.

$ cat test.py
import re

pre = r"@conda-forge-(admin|linter)\s*[,:]?\s*"
ADD_USER = re.compile(pre + r"(please )?add user @(?P<user>\S+)$", re.I)

tests = [
    "@conda-forge-admin, please add user @My-NAME",
    "@conda-forge-admin please add user @My-NAME"
]

for text in tests:
    match = ADD_USER.search(text)
    print(match.group('user'))
$ python test.py
My-NAME
My-NAME

@beckermr
Copy link
Member

This appears to have fixed itself and IDK why. Let's close for now.

@jakirkham
Copy link
Member

Saw this again with issue ( conda-forge/benchling-sdk-feedstock#11 ). Have reopened to track

@beckermr
Copy link
Member

Yeah this is weird. I tested extensively while watching the webserver logs and could not reproduce. Arghhhh. Thanks for reopening.

@jakirkham
Copy link
Member

No worries. It's an odd issue. Don't recall there being lots of changes to relevant code paths here, which makes it a bit confusing

Is it possible there's an issue on GitHub's end somehow? TBH I don't know how this would happen. Just trying to come up with other ideas

@beckermr
Copy link
Member

That is possible. It's hard, we if we can find the webhook event in the settings pane for conda-forge, it's possible to resend it to debug love.

@jakirkham
Copy link
Member

Adding another data point, saw this with re-rendering issue request recently ( conda-forge/shellcheck-feedstock#12 )

@jakirkham jakirkham changed the title Documented issue title for adding a feedstock maintainer does not trigger the conda-forge-admin automation Bot commands in new issues sensitive to , after bot user name Feb 23, 2023
@ap--
Copy link
Member

ap-- commented Mar 21, 2023

I think this might have the same issue: conda-forge/universal_pathlib-feedstock#26

@jakirkham
Copy link
Member

Yeah try deleting the ,

@jakirkham
Copy link
Member

Fun fact: Adding a <space> before the , also works ( conda-forge/universal_pathlib-feedstock#26 (comment) ). Wonder if there is some kind of filtering GH is doing with say invalid GH user names (maybe as a form of sanitization to protect against other issues 🤔)?

@beckermr
Copy link
Member

I think it is the fact that the command is run twice.

@jakirkham
Copy link
Member

Maybe

Went ahead and tried including the <space> to start and it seemed to work ( conda-forge/openmpi-feedstock#120 )

Suppose it could just be luck though. We could try more tests

@jaimergp
Copy link
Member

jaimergp commented Jul 6, 2024

One thing I noticed while debugging the 🚀 reacts to the commands is that the new-issue event payload is sent before the API can return the issue content. So we need to re-run the issue fetching logic a in a tiny loop. This is done in one more part in the code, IIRC. See:

for i in range(NUM_GH_API_TRIES):
try:
comment = _find_reactable_comment(
repo, issue_number, comment_id, review_id
)
break
except RuntimeError as exc:
# There seems to be a race condition where we get the payload before the
# API can return the actual comment, so let's retry for a tiny bit
if i < 4:
time.sleep(0.050 * 2**i)
continue
raise exc
comment.create_reaction(reaction)

And:

# sometimes the webhook outpaces other bits of the API so we try a bit
for i in range(NUM_GH_API_TRIES):
try:
# this token has to be that of an actual bot since we use this
# to make a fork
# the bot used does not need admin permissions
gh = github.Github(os.environ["GH_TOKEN"])
repo = gh.get_repo("{}/{}".format(org_name, repo_name))
default_branch = repo.default_branch
break
except Exception as e:
if i < 4:
time.sleep(0.050 * 2**i)
continue
else:
raise e

@beckermr
Copy link
Member

beckermr commented Jul 6, 2024

Yes a race condition makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

5 participants