softdevteam / mattermost-github-integration

GitHub integration for Mattermost
Other
79 stars 50 forks source link

add basic support for pull request 'synchronize' action #43

Closed lfbayer closed 6 years ago

lfbayer commented 6 years ago

I noticed that there is no notification when a user adds a commit to a pull request. This pull request addresses that with a very trivial notification for when the "synchronize" action for pull_request is received.

The message will look like this:

lfbayer modified pull request #43 add basic support for pull request 'synchronize' action

ptersilie commented 6 years ago

Oh are these not sent as normal commits and show up as

user pushed 1 changeset to branch at repo

If not, does the synchronize payload contain the commits? Then we could add them to the synchronize event:

lfbayer modified pull request #43 add basic support for pull request 'synchronize' action: Commit 1: A commit Commit 2: Another commit

lfbayer commented 6 years ago

We do not get the notifications for the commits themselves, because for a pull request those commits are in a different repository that is not sending the webhook event.

As far as I can tell the synchronize payload does not contain the commits, because the synchronize is just a notification that the pull request is now pointed at a different commit. It does contain a before and an after commit.

ptersilie commented 6 years ago

Ah I see. That's probably why I haven't noticed this yet, because all our repos are on the same account. But I see how this is a problem for your setup, so I'll gladly merge this.

lfbayer commented 6 years ago

Thanks for merging this!

It would be nice to see the list of all the commits, but it looked like that would require another trip to the git repository to figure out what those commits were.

And there is also the possibility that the change was a rebase, which would mean that some commits are removed as well, so I wasn't sure what that should look like in the notification. For now I thought the simple notification was sufficient to just let us know that at least something changed.

ptersilie commented 6 years ago

Sounds reasonable. If you ever figure out how to improve on this, just make another PR and I'm happy to merge that in as well.