conda-forge / conda-forge-webservices

the heroku app deployed to run conda-forge admin commands and linting
BSD 3-Clause "New" or "Revised" License
8 stars 33 forks source link

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

Open kbrindley opened 1 year ago

kbrindley commented 1 year ago

Solution to issue cannot be found in 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

jakirkham commented 1 year ago

Saw this again with issue ( https://github.com/conda-forge/benchling-sdk-feedstock/issues/11 ). Have reopened to track

beckermr commented 1 year ago

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

jakirkham commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Adding another data point, saw this with re-rendering issue request recently ( https://github.com/conda-forge/shellcheck-feedstock/issues/12 )

ap-- commented 1 year ago

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

jakirkham commented 1 year ago

Yeah try deleting the ,

jakirkham commented 1 year ago

Fun fact: Adding a <space> before the , also works ( https://github.com/conda-forge/universal_pathlib-feedstock/issues/26#event-8810897231 ). 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 commented 1 year ago

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

jakirkham commented 1 year ago

Maybe

Went ahead and tried including the <space> to start and it seemed to work ( https://github.com/conda-forge/openmpi-feedstock/issues/120 )

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

jaimergp commented 2 months ago

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:

https://github.com/conda-forge/conda-forge-webservices/blob/9978abd1e8b84fda67cbb65b37f8143489a23a86/conda_forge_webservices/commands.py#L108-L121

And:

https://github.com/conda-forge/conda-forge-webservices/blob/9978abd1e8b84fda67cbb65b37f8143489a23a86/conda_forge_webservices/commands.py#L356-L371

beckermr commented 2 months ago

Yes a race condition makes sense.