-
Notifications
You must be signed in to change notification settings - Fork 148
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
[MM-352] Add feature to publish release create and delete events #762
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍
A few questions below
@@ -95,7 +95,7 @@ As a system admin, you must create a webhook for each organization you want to r | |||
- **Content Type:** `application/json` | |||
- **Secret:** the webhook secret you copied previously. | |||
6. Select **Let me select individual events** for "Which events would you like to trigger this webhook?". | |||
7. Select the following events: `Branch or Tag creation`, `Branch or Tag deletion`, `Issue comments`, `Issues`, `Pull requests`, `Pull request review`, `Pull request review comments`, `Pushes`, `Stars`. | |||
7. Select the following events: `Branch or Tag creation`, `Branch or Tag deletion`, `Issue comments`, `Issues`, `Pull requests`, `Pull request review`, `Pull request review comments`, `Pushes`, `Stars`, `Releases`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changelog should mention that GH admins need to update their hooks.
@ayusht2810 Do you have a suggestion where we could store that notice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hanzei We can manually update release notes to have that changelog or create a separate PR for this change to have a changelog for it. What is your opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release notes are essentially the changelog for plugin releases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have Branch or Tag creation
already. Isn't that about the same as releases? I'm actually not sure if that tag creation feature works properly
{{- if eq .GetAction "created" }} created a release {{template "release" .GetRelease}} | ||
{{- else if eq .GetAction "deleted" }} deleted a release {{template "release" .GetRelease}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other actions besides these two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the other events are:
- Release created
- Release edited
- Release published
- Release unpublished
- Release deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, other than @hanzei comments above
@@ -95,7 +95,7 @@ As a system admin, you must create a webhook for each organization you want to r | |||
- **Content Type:** `application/json` | |||
- **Secret:** the webhook secret you copied previously. | |||
6. Select **Let me select individual events** for "Which events would you like to trigger this webhook?". | |||
7. Select the following events: `Branch or Tag creation`, `Branch or Tag deletion`, `Issue comments`, `Issues`, `Pull requests`, `Pull request review`, `Pull request review comments`, `Pushes`, `Stars`. | |||
7. Select the following events: `Branch or Tag creation`, `Branch or Tag deletion`, `Issue comments`, `Issues`, `Pull requests`, `Pull request review`, `Pull request review comments`, `Pushes`, `Stars`, `Releases`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have Branch or Tag creation
already. Isn't that about the same as releases? I'm actually not sure if that tag creation feature works properly
@mickmister I tested the tag feature by pushing a tag on a test repo, and I was able to get the message for the pushed tag.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above PR has been tested for the following scenarios:-
- Notification for creating a release
- Notification for deleting a release
The PR was working fine for both the conditions, LGTM. Approved.
* update github docs for mattermost/mattermost-plugin-github#762 * update zoom plugin docs for mattermost/mattermost-plugin-zoom#375 * improvements to github plugin page * remove todo comment --------- Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com>
Summary
Screenshot
Ticket Link
Fixes #723
What to test?
Checklist
make test
Ran test cases and ensured they are passingmake check-style
Ran style check and ensured both webapp and server pass the checks