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

Don't react to PR comments #110

Merged
merged 1 commit into from
Nov 27, 2019
Merged

Don't react to PR comments #110

merged 1 commit into from
Nov 27, 2019

Conversation

foolip
Copy link
Member

@foolip foolip commented Oct 29, 2019

This simplifies the filtering and removes the need to fetch the pull
request in getPullRequest.

@foolip
Copy link
Member Author

foolip commented Oct 29, 2019

The code changes here are probably straightforward enough to review, but the key concern here should be unintended side effects of this change:

  • Could the code be broken in such a way that comments were previously the only thing that triggered the right things, given that a PR description counts as a comment?
  • Is wpt-pr-bot reliable, and if not is that effectively masked by reacting to comments also?

@foolip foolip requested a review from jugglinmike October 29, 2019 14:38
@jugglinmike
Copy link
Contributor

I like this simplification! We should also reconfigure the GitHub Webhook by removing the "Issues" and "Issue comments" events. In that case, I'd push to remove the explicit filtering (since that would be dead code, and it would insinuate the possibility of unreachable states).

  • Could the code be broken in such a way that comments were previously the only thing that triggered the right things, given that a PR description counts as a comment?
  • Is wpt-pr-bot reliable, and if not is that effectively masked by reacting to comments also?

I'm not aware of any problems like that, but I can't say they don't exist. We could build some confidence by inspecting "timelines" from recent pull requests, looking for places where the script acted directly after a comment.

@zcorpan
Copy link
Member

zcorpan commented Oct 31, 2019

For #33 and #81 the bot may need to listen for new comments again, unless we use labels instead to communicate with the bot.

@foolip
Copy link
Member Author

foolip commented Oct 31, 2019

Yes, that's a good point. Perhaps that could then be isolated from the code that reacts to PR creation/update, though? It was quite hard to follow the code as it was here.

@zcorpan
Copy link
Member

zcorpan commented Oct 31, 2019

Perhaps that could then be isolated from the code that reacts to PR creation/update, though?

It probably could, yeah.

@jugglinmike
Copy link
Contributor

Making this script readable and testable is a worthy goal, especially if we're intending to continue extending it. I think that will require a more dedicated effort, though. It'd be great to bite the bullet for the technical debt and move forward with confidence.

@foolip
Copy link
Member Author

foolip commented Oct 31, 2019

I still see #77 as plan A, certainly for adding labels and reviewers. @zcorpan's upcoming work on improving the review process might reveal some needs that we can't do with GitHub Actions and maybe we'll have to keep operating this bot, but most of the code that exists I'm not confident will continue to live in this repo. That is to say, improving as we make changes is a good idea, but wrangling it all under control could well turn out to be a wasted effort.

@foolip foolip mentioned this pull request Nov 8, 2019
@foolip
Copy link
Member Author

foolip commented Nov 12, 2019

I believe this will fix #69. But it might be a good idea to merge #118 and add some minimal test coverage for index.js first.

@foolip foolip requested review from zcorpan and removed request for jugglinmike November 14, 2019 15:16
@foolip
Copy link
Member Author

foolip commented Nov 14, 2019

@zcorpan per discussion with @boazsender I'm reassigning this and two other of my PRs to you. Not super high priority but would be nice to get it done within a few weeks.

This simplifies the filtering and removes the need to fetch the pull
request in `getPullRequest`.
@zcorpan zcorpan force-pushed the foolip/ignore-comments branch from 4b67d3c to a358350 Compare November 27, 2019 13:24
@zcorpan zcorpan merged commit f55e575 into master Nov 27, 2019
zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Nov 27, 2019
@zcorpan
Copy link
Member

zcorpan commented Nov 27, 2019

In web-platform-tests/wpt#20476 the comment produced this in the papertrail

Nov 27 13:33:51 wpt-pr-bot app/web.1 Ignoring event: not a pull request

@foolip foolip deleted the foolip/ignore-comments branch November 27, 2019 15:02
@foolip
Copy link
Member Author

foolip commented Nov 27, 2019

@zcorpan sweet, thanks! Does everything still seem to be working for a new and updated PR?

@zcorpan
Copy link
Member

zcorpan commented Nov 27, 2019

Yes, I think so. I haven't tested updating PRs, but the papertrail shows recent activity for e.g. web-platform-tests/wpt#20441

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

Successfully merging this pull request may close these issues.

3 participants