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

Conversation

ayusht2810
Copy link
Contributor

@ayusht2810 ayusht2810 commented Apr 8, 2024

Summary

  • Added feature to publish workflow failure events

Screenshot

image

Ticket Link

Fixes #722
Fixes #565

What to test?

  • Update the webhook for the repo to receive notifications for workflow events
  • Create a subscription for a repo with a subscription enabled for workflow events
  • Create workflow in which a particular jobs fail
  • Check for the event received in the subscribed channel

Checklist

  • Completed dev testing
  • make test Ran test cases and ensured they are passing
  • make check-style Ran style check and ensured both webapp and server pass the checks

@ayusht2810 ayusht2810 self-assigned this Apr 8, 2024
@ayusht2810 ayusht2810 added the 2: Dev Review Requires review by a core committer label Apr 8, 2024
Comment on lines +1412 to +1415
for _, sub := range subs {
if !sub.Workflows() {
continue
}
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.

server/plugin/template.go Outdated Show resolved Hide resolved
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.
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

@mickmister mickmister changed the title [MM-356] Add feature to publish workflow failure events [MM-356] Add feature to subscribe to release and workflow events Jun 20, 2024
Copy link
Member

@mickmister mickmister left a 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

go.mod Outdated Show resolved Hide resolved
server/plugin/command.go Outdated Show resolved Hide resolved
webapp/package.json Outdated Show resolved Hide resolved
return
}

newReleaseMessage, err := renderTemplate("newReleaseEvent", event)
Copy link
Member

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

server/plugin/template.go Outdated Show resolved Hide resolved
@Kshitij-Katiyar
Copy link
Contributor

@mickmister Fixed the comments, Please have a look.

server/plugin/template_test.go Show resolved Hide resolved
webapp/package-lock.json Outdated Show resolved Hide resolved
@wiggin77
Copy link
Member

wiggin77 commented Aug 1, 2024

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

@raghavaggarwal2308
Copy link
Contributor

@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.

@wiggin77
Copy link
Member

wiggin77 commented Aug 6, 2024

@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.

Copy link
Member

@wiggin77 wiggin77 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@wiggin77 wiggin77 added the 3: QA Review Requires review by a QA tester label Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Subscribe to GitHub Action workflow failures Support for all github webhook events
6 participants