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

[MM-356] Add feature to subscribe to release and workflow events #765

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`, `Workflow jobs`.
7. Hit **Add Webhook** to save it.

If you have multiple organizations, repeat the process starting from step 3 to create a webhook for each organization.
Expand Down Expand Up @@ -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.
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor

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

Copy link
Member

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?

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hanzei Since the plugin functionality details have been moved to mattermost docs repo, I have made a PR on it for the same

- `--exclude-org-member`: events triggered by organization members will not be delivered. It will be locked to the organization provided in the plugin configuration and it will only work for users whose membership is public. Note that organization members and collaborators are not the same.
- `--render-style`: notifications will be delivered in the specified style (for example, the body of a pull request will not be displayed). Supported
values are `collapsed`, `skip-body` or `default` (same as omitting the flag).
Expand Down
4 changes: 3 additions & 1 deletion server/plugin/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const (
featureIssueComments = "issue_comments"
featurePullReviews = "pull_reviews"
featureStars = "stars"
featureWorkflows = "workflows"
)

const (
Expand All @@ -44,6 +45,7 @@ var validFeatures = map[string]bool{
featureIssueComments: true,
featurePullReviews: true,
featureStars: true,
featureWorkflows: true,
}

type Features string
Expand Down Expand Up @@ -894,7 +896,7 @@ func getAutocompleteData(config *Configuration) *model.AutocompleteData {

subscriptionsAdd := model.NewAutocompleteData("add", "[owner/repo] [features] [flags]", "Subscribe the current channel to receive notifications about opened pull requests and issues for an organization or repository. [features] and [flags] are optional arguments")
subscriptionsAdd.AddTextArgument("Owner/repo to subscribe to", "[owner/repo]", "")
subscriptionsAdd.AddNamedTextArgument("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", "", `/[^,-\s]+(,[^,-\s]+)*/`, false)
subscriptionsAdd.AddNamedTextArgument("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", "", `/[^,-\s]+(,[^,-\s]+)*/`, false)

if config.GitHubOrg != "" {
subscriptionsAdd.AddNamedStaticListArgument("exclude-org-member", "Events triggered by organization members will not be delivered (the organization config should be set, otherwise this flag has not effect)", false, []model.AutocompleteListItem{
Expand Down
4 changes: 4 additions & 0 deletions server/plugin/subscriptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,10 @@ func (s *Subscription) Stars() bool {
return strings.Contains(s.Features.String(), featureStars)
}

func (s *Subscription) Workflows() bool {
return strings.Contains(s.Features.String(), featureWorkflows)
}

func (s *Subscription) Label() string {
if !strings.Contains(s.Features.String(), "label:") {
return ""
Expand Down
21 changes: 21 additions & 0 deletions server/plugin/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,16 @@ func init() {
return commit.GetCommitter()
}

funcMap["workflowJobFailedStep"] = func(steps []*github.TaskStep) string {
for _, step := range steps {
if step.GetConclusion() == workflowJobFail {
return step.GetName()
}
}

return ""
}

masterTemplate = template.Must(template.New("master").Funcs(funcMap).Parse(""))

// The user template links to the corresponding GitHub user. If the GitHub user is a known
Expand Down Expand Up @@ -158,6 +168,11 @@ func init() {
`[#{{.GetNumber}} {{.GetTitle}}]({{.GetHTMLURL}})`,
))

// The workflow job links to the corresponding workflow.
template.Must(masterTemplate.New("workflowJob").Parse(
`[{{.GetName}}]({{.GetHTMLURL}})`,
))

// The eventRepoIssue links to the corresponding issue. Note that, for some events, the
// issue *is* a pull request, and so we still use .GetIssue and this template accordingly.
template.Must(masterTemplate.New("eventRepoIssue").Parse(
Expand Down Expand Up @@ -408,6 +423,7 @@ Assignees: {{range $i, $el := .Assignees -}} {{- if $i}}, {{end}}{{template "use
" * `issue_comments` - includes new issue comments\n" +
" * `issue_creations` - includes new issues only \n" +
" * `pull_reviews` - includes pull request reviews\n" +
" * `workflows` - includes workflow job failures\n" +
" * `label:<labelname>` - limit pull request and issue events to only this label. Must include `pulls` or `issues` in feature list when using a label.\n" +
" * Defaults to `pulls,issues,creates,deletes`\n\n" +
" * `--exclude-org-member` - events triggered by organization members will not be delivered (the GitHub organization config should be set, otherwise this flag has not effect)\n" +
Expand All @@ -429,6 +445,11 @@ Assignees: {{range $i, $el := .Assignees -}} {{- if $i}}, {{end}}{{template "use
{{- else }} unstarred
{{- end }} by {{template "user" .GetSender}}
It now has **{{.GetRepo.GetStargazersCount}}** stars.`))

template.Must(masterTemplate.New("newWorkflowJob").Funcs(funcMap).Parse(`
{{template "repo" .GetRepo}} {{template "workflowJob" .GetWorkflowJob}} workflow got failed (triggered by {{template "user" .GetSender}})
ayusht2810 marked this conversation as resolved.
Show resolved Hide resolved
Step failed: {{.GetWorkflowJob.Steps | workflowJobFailedStep}}
Commit: {{.GetRepo.GetHTMLURL}}/commit/{{.GetWorkflowJob.GetHeadSHA}}`))
}

func registerGitHubToUsernameMappingCallback(callback func(string) string) {
Expand Down
37 changes: 37 additions & 0 deletions server/plugin/template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1445,6 +1445,43 @@ func TestGitHubUsernameRegex(t *testing.T) {
}
}

func TestWorkflowJobNotification(t *testing.T) {
Kshitij-Katiyar marked this conversation as resolved.
Show resolved Hide resolved
t.Run("failed", func(t *testing.T) {
expected := `
[\[mattermost-plugin-github\]](https://github.com/mattermost/mattermost-plugin-github) [mock-workflow-job](https://github.com/mattermost/mattermost-plugin-github/actions/runs/12345/job/67890) workflow got failed (triggered by [panda](https://github.com/panda))
Step failed: mock-job-2
Commit: https://github.com/mattermost/mattermost-plugin-github/commit/1234567890`

actual, err := renderTemplate("newWorkflowJob", &github.WorkflowJobEvent{
Repo: &repo,
Sender: &user,
Action: sToP(actionCompleted),
WorkflowJob: &github.WorkflowJob{
Conclusion: sToP("failure"),
Name: sToP("mock-workflow-job"),
HeadSHA: sToP("1234567890"),
HTMLURL: sToP("https://github.com/mattermost/mattermost-plugin-github/actions/runs/12345/job/67890"),
Steps: []*github.TaskStep{
{
Name: sToP("mock-job-1"),
Conclusion: sToP("success"),
},
{
Name: sToP("mock-job-2"),
Conclusion: sToP("failure"),
},
{
Name: sToP("mock-job-3"),
Conclusion: sToP("success"),
},
},
},
})
require.NoError(t, err)
require.Equal(t, expected, actual)
})
}

func sToP(s string) *string {
return &s
}
Expand Down
55 changes: 52 additions & 3 deletions server/plugin/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,12 @@ const (
actionLabeled = "labeled"
actionAssigned = "assigned"

actionCreated = "created"
actionDeleted = "deleted"
actionEdited = "edited"
actionCreated = "created"
actionDeleted = "deleted"
actionEdited = "edited"
actionCompleted = "completed"

workflowJobFail = "failure"

postPropGithubRepo = "gh_repo"
postPropGithubObjectID = "gh_object_id"
Expand Down Expand Up @@ -282,6 +285,11 @@ func (p *Plugin) handleWebhook(w http.ResponseWriter, r *http.Request) {
handler = func() {
p.postStarEvent(event)
}
case *github.WorkflowJobEvent:
repo = event.GetRepo()
handler = func() {
p.postWorkflowJobEvent(event)
}
}

if handler == nil {
Expand Down Expand Up @@ -1377,3 +1385,44 @@ func (p *Plugin) postStarEvent(event *github.StarEvent) {
}
}
}

func (p *Plugin) postWorkflowJobEvent(event *github.WorkflowJobEvent) {
if event.GetAction() != actionCompleted {
return
}

// Create a post only when the workflow job is completed and fails
if event.GetWorkflowJob().GetConclusion() != workflowJobFail {
return
}

repo := event.GetRepo()
subs := p.GetSubscribedChannelsForRepository(repo)

if len(subs) == 0 {
return
}

newWorkflowJobMessage, err := renderTemplate("newWorkflowJob", event)
if err != nil {
p.client.Log.Warn("Failed to render template", "Error", err.Error())
return
}

for _, sub := range subs {
if !sub.Workflows() {
continue
}
Comment on lines +1374 to +1377
Copy link
Member

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?

Copy link
Contributor

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?

Copy link
Contributor Author

@ayusht2810 ayusht2810 May 1, 2024

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?

Copy link
Contributor Author

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.

Copy link
Member

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.


post := &model.Post{
UserId: p.BotUserID,
Type: "custom_git_workflow_job",
Message: newWorkflowJobMessage,
ChannelId: sub.ChannelID,
}

if err = p.client.Post.CreatePost(post); err != nil {
p.client.Log.Warn("Error webhook post", "Post", post, "Error", err.Error())
}
}
}
Loading