Also show warnings when PRs are updated

[Urgau]

This PR tweaks the assign handler to also show warnings when PRs are updated, instead of just doing it when a PR is opened.

Fixes #1831
r? @ehuss

[ehuss]

This looks like it will warn on every push, is that right? If possible I'd like to avoid that (maybe with IssueData?).

[Urgau]

That's a good observation, it would indeed post unnecessary comments.

As suggested I have used IssueData with some state to keep track of the last warnings to avoid posting unnecessary comments. I haven't tested it, but it's very similar to the other avoid comment logic.

[Kobzol]

Hmm, it's quite weird in the first place that a handler called assign is handling warnings about modified submodules 😅 Since triagebot doesn't try to send the welcome/assignment and warning messages in a single comment, I think that we should split the logic here.

Keep welcoming and initial assignment in the assign.rs handler (which will still only run on Open), and then create a separate handler for checking the commits of a PR (that would run on Open and Synchronize), that would check:

• submodules
• merge commits
• mentions

As I proposed here. What do you think?

[Urgau]

I agree it would be good to split the submodule warning logic outside of the assign handler. I think it was done their out of convenience since their is already a diff operation going on inside it.

As for a single "commits" handler I fear it will become too complex quickly with the different pre-conditions of each handler. What about keeping them as different handlers but with a simple lazy loading of the commits, that way each handler continues to do it's own thing and we don't unnecessarily fetch the commits multiple times.

As for the submodules warnings it only works only on the PR diff, not commits. We could also create a lazy loading of the diff.

[Kobzol]

I think that we could do a single "check_commits" handler that would have several sub-responsibilities (checking submodules, mentions and merge commits), that could also make it more discoverable to figure out where we check commits (I certainly wouldn't look for submodule changes in the assign handler, lol). It's not so much about the used API (diff vs commit list), but rather about related functionality, which could live at a single place. It would also help with removing some of the duplication of PR status checks (e.g. draft and PR title). It's weird if checking merge commits doesn't trigger when you have a WIP PR title, but no_mentions triggers in that situation, that's quite inconsistent..

On the other hand, that would be a bit of a larger change. We can just keep the status quo, as always.

In terms of performance, rather than doing lazy loading, I would create some "per-event context" that is filled when the various handlers are executed. In other words, when handling a single event and threading it through many handlers, the first time something calls ".commits()", they will be loaded from GH and cached. The second time something calls ".commits()", they would be loaded from memory. If we only keep the cache for a specific GH event and don't persist it further, there should be no invalidation problems (it would even help keep the behavior of the individual handlers more consistent, as in theory the GH state can change in between the execution of two handlers on the same webhook event).

(Edit: the caching above is not blocking for this or no_mentions, it's just an idea. In practice the perf. will probably be fine.)

[Urgau]

Sure, seems fine to parent handler which would have many sub-responsibility, it would even share the logic for printing the warnings, hidden them, checking the draft status, ... I like that.

In terms of how to proceed, what about?

1. Creation of the check_commits handler and moving of "submodules" and "non-default-branch"
2. Rebase this PR to be in the check_commits handler (their by introducing hidding and synchronize)
3. Move the "merge commits" checks into check_commits handler
4. Add the "mentions" checks within the check_commits handler (tbd in Add no GitHub mentions in commits handler #1917)
5. Add caching/per-event context

[Kobzol]

I didn't want to block this functionality on that refactoring, but I'm willing to review it quickly, and help with the implementation if needed, so hopefully it shouldn't take long. I'm also working now on extending the test suite to give us the ability to actually test the issue handlers, but that does not need to block this refactor.

That plan sounds good!

[Urgau]

Rebased on top of #1922, the logic is now in the check_commits handler.

[Kobzol]

For warn_non_default_branch, we should also be checking the PR edited event, but I don't think that's super important, as it should be very rare to "repoint" a PR to a different base branch after it has been created. The important event is Synchronize for the submodules, because that happens quite often, so it would be nice to warn about it.