-
Notifications
You must be signed in to change notification settings - Fork 152
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-356] Add feature to subscribe to release and workflow events #765
Conversation
for _, sub := range subs { | ||
if !sub.Workflows() { | ||
continue | ||
} |
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.
1/5 I think we should support (or require) a workflow
parameter here (and maybe support job
parameter as well). Maybe we can support both individual workflows and all workflow cases:
Just the ci
workflow
/github subscriptions add owner/repo pulls,workflows:ci,issues
All workflows
/github subscriptions add owner/repo pulls,workflows:*,issues
@hanzei What do you think?
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.
Makes sense to me 👍
Limiting the notifications to a specific branch seems like a common use case. Can we include that in there as well?
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.
Just the ci workflow
@mickmister Are you suggesting here to add a subscription on the basis of the workflow name, which is ci
in the above case, or have I misunderstood the comment?
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.
Limiting the notifications to a specific branch seems like a common use case. Can we include that in there as well?
@mickmister @hanzei For the above case, what should be the slash command format? Should we keep something like: /github subscriptions add owner/repo pulls,workflows:branch-b1,b2;name-n1,n2;job-j1,j2
? Please let me know if you have any better suggestions for the above case.
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 subscriptions add
feature is quite overloaded at this point. Adding more complexity on top of it may not be the best thing to do. Created a ticket for breaking this out of the command #780
Mainly thinking of the complexity of the job
parameter there. I'm thinking we should avoid the job
parameter, since those jobs may not belong to all workflows. Then it's up to the actions workflow developer in that project to structure the workflows in a way to be something to subscribe to.
README.md
Outdated
@@ -150,7 +150,7 @@ When you’ve tested the plugin and confirmed it’s working, notify your team s | |||
/github subscriptions add mattermost/mattermost-server --features issues,pulls,issue_comments,label:"Help Wanted" | |||
``` | |||
- The following flags are supported: | |||
- `--features`: comma-delimited list of one or more of: issues, pulls, pulls_merged, pulls_created, pushes, creates, deletes, issue_creations, issue_comments, pull_reviews, label:"labelname". Defaults to pulls,issues,creates,deletes. | |||
- `--features`: comma-delimited list of one or more of: issues, pulls, pulls_merged, pulls_created, pushes, creates, deletes, issue_creations, issue_comments, pull_reviews, workflows, label:"labelname". Defaults to pulls,issues,creates,deletes. |
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.
I wonder if we should rename the flag to workflow-failures
to make it clear that only these get notifications. Or add an option to workflows
that specifies if only failed or also successful runs should trigger a notification.
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.
Updated the flag to workflow-failures
. Let me know if I should do the latter one.
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.
I'm thinking we should have separate workflow-success
and workflow-failure
subscription types
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.
@mickmister Added the workflow_success
. Please have a look
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 are you still requesting changes here?
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.
Where did the README.md
updates go?
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.
@Kshitij-Katiyar Great job on this 👍 I have a few more comments
server/plugin/webhook.go
Outdated
return | ||
} | ||
|
||
newReleaseMessage, err := renderTemplate("newReleaseEvent", event) |
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.
@phoinixgrr FYI this feature in the GitHub plugin will allow us to create subscriptions to new releases
@mickmister Fixed the comments, Please have a look. |
This PR is going to have a significant merge conflict with https://github.com/mattermost/mattermost-plugin-github/pull/808/files Does anyone have an opinion on which is more important?: release and workflow subscriptions vs discussions and comment |
@wiggin77 In both the PRs we mainly adding new functions not updating the existing functions much. I think it won't be difficult to fix the conflicts in this case. So, we can merge whichever PR is ready first. Please let me know your opinions on this. |
Ok, let's get this one merged first. |
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 👍
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
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.
Tested this PR for both success and failure scenarios in the workflow and the user is receiving the message. LGTM.
Summary
Screenshot
Ticket Link
Fixes #722
Fixes #565
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