MM-67473 Adding support to workflow run success/failures#975
Open
avasconcelos114 wants to merge 8 commits intomasterfrom
Open
MM-67473 Adding support to workflow run success/failures#975avasconcelos114 wants to merge 8 commits intomasterfrom
avasconcelos114 wants to merge 8 commits intomasterfrom
Conversation
avasconcelos114
added
2: Dev Review
Contributor
Author
|
@coderabbitai please review this PR 🐇 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds workflow-run notifications: new feature flags for run success/failure, subscription predicates, a workflow-run template, webhook handler to process completed runs, test utilities, and tests validating rendering and posting behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor GitHub
participant Webhook as Plugin Webhook
participant Store as Subscription Store
participant Template as Template Engine
participant API as Mattermost API
GitHub->>Webhook: Send WorkflowRunEvent (action: completed)
Webhook->>Webhook: postWorkflowRunEvent()
Webhook->>Store: Query subscriptions for repo/org
Store-->>Webhook: Return subscriptions
Webhook->>Store: Filter subscriptions by WorkflowRunFailures/Successes and membership rules
Store-->>Webhook: Matching subscriptions
loop For each matching subscription
Webhook->>Template: Render workflowRunCompleted with event data
Template-->>Webhook: Rendered message
Webhook->>API: Create post (custom_git_workflow_run) in channel
API-->>Webhook: Post result / error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'modernize', run 'golangci-lint help linters' to see the list of supported linters Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/plugin/webhook.go`:
- Around line 1505-1529: The workflow-run notification loop (iterating subs and
using sub.WorkflowRunFailures()/sub.WorkflowRunSuccesses()) currently skips
applying org-member subscription filters; update the loop to check each
subscription's org-member policy (e.g., methods like sub.ExcludeOrgMembers() and
sub.IncludeOnlyOrgMembers()) against the event actor/org membership flag (e.g.,
event.ActorIsOrgMember or equivalent on the event payload) before
rendering/sending posts; if the subscription excludes org members and the actor
is an org member skip, and if the subscription is include-only and the actor is
not an org member skip, then proceed to renderTemplate("workflowRunCompleted",
event) and call p.client.Post.CreatePost(post).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/plugin/command.goserver/plugin/subscriptions.goserver/plugin/template.goserver/plugin/template_test.goserver/plugin/webhook.go
Contributor
Author
|
@coderabbitai Please review again and either request more changes or approve the PR |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 60811 🏁 Script executed: Length of output: 40602 🏁 Script executed: Length of output: 26687 🏁 Script executed: Length of output: 55881
✅ Code QualityStrengths:
Note: The AI summary incorrectly flagged a duplicate test function, but verification shows only one 🎯 Implementation Highlights
No issues found. The code is production-ready! [approve] |
marianunez
requested changes
Mar 4, 2026
server/plugin/webhook.go
Outdated
Comment on lines
+1518
to
+1523
| if workflowRunMessage == "" { | ||
| var err error | ||
| workflowRunMessage, err = renderTemplate("workflowRunCompleted", event) | ||
| if err != nil { | ||
| p.client.Log.Warn("Failed to render template", "Error", err.Error()) | ||
| return |
Member
There was a problem hiding this comment.
Should this be continue instead of return so that it finishes processing all the subscriptions despite that template error on a specific one?
Member
There was a problem hiding this comment.
Looking at this more, I see we just have one template for all so we should probably try to get the workflowRunMessage once before this iteration loop so that it removes that extra check of the message each time that its processing the subs and is more readable as well
Contributor
Author
There was a problem hiding this comment.
Ahh absolutely yes that is much cleaner, I'll fix that up
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/plugin/webhook_test.go (1)
1200-1367: Strong table-driven coverage; addcancelledandtimed_outrouting cases.Given
workflow_run_failureis intended to cover failures/cancellations/timeouts, add explicit cases forcancelledandtimed_outto lock behavior and prevent regressions.🧪 Suggested additions
{ name: "successful workflow run failure notification", event: GetMockWorkflowRunEvent(actionCompleted, "failure", MockRepo, MockOrg, MockSender), setup: func() { ... }, }, + { + name: "successful workflow run cancelled notification", + event: GetMockWorkflowRunEvent(actionCompleted, "cancelled", MockRepo, MockOrg, MockSender), + setup: func() { + mockKvStore.EXPECT().Get("subscriptions", mock.MatchedBy(func(val any) bool { + _, ok := val.(**Subscriptions) + return ok + })).DoAndReturn(setupMockSubscriptions(map[string][]*Subscription{ + "mockrepo/mockorg": { + {ChannelID: MockChannelID, CreatorID: MockCreatorID, Features: Features(featureWorkflowRunFailure), Repository: MockRepo}, + }, + })).Times(1) + mockAPI.On("CreatePost", mock.Anything).Return(&model.Post{}, nil).Times(1) + }, + }, + { + name: "successful workflow run timed_out notification", + event: GetMockWorkflowRunEvent(actionCompleted, "timed_out", MockRepo, MockOrg, MockSender), + setup: func() { + mockKvStore.EXPECT().Get("subscriptions", mock.MatchedBy(func(val any) bool { + _, ok := val.(**Subscriptions) + return ok + })).DoAndReturn(setupMockSubscriptions(map[string][]*Subscription{ + "mockrepo/mockorg": { + {ChannelID: MockChannelID, CreatorID: MockCreatorID, Features: Features(featureWorkflowRunFailure), Repository: MockRepo}, + }, + })).Times(1) + mockAPI.On("CreatePost", mock.Anything).Return(&model.Post{}, nil).Times(1) + }, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin/webhook_test.go` around lines 1200 - 1367, Add two table-driven test cases inside TestPostWorkflowRunEvent to assert that workflow runs with conclusions "cancelled" and "timed_out" are routed the same as failures: use GetMockWorkflowRunEvent(actionCompleted, "cancelled", ...) and GetMockWorkflowRunEvent(actionCompleted, "timed_out", ...), set up mockKvStore to return a subscription that includes Features(featureWorkflowRunFailure) (same pattern as the existing "successful workflow run failure notification" case), and expect mockAPI.CreatePost to be called once; ensure the test names are unique (e.g., "workflow run cancelled notification" and "workflow run timed_out notification") so TestPostWorkflowRunEvent covers these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/plugin/test_utils.go`:
- Around line 430-448: The test fixture GetMockWorkflowRunEvent builds FullName
and HTML URLs as "repo/org" which is reversed; update FullName, the repository
HTMLURL and the WorkflowRun.HTMLURL to use "org/repo" ordering (swap the repo
and org arguments in the fmt.Sprintf calls that build FullName, Repo.HTMLURL and
WorkflowRun.HTMLURL) so the fixture matches real GitHub payloads and avoids
masking subscription/URL rendering bugs.
---
Nitpick comments:
In `@server/plugin/webhook_test.go`:
- Around line 1200-1367: Add two table-driven test cases inside
TestPostWorkflowRunEvent to assert that workflow runs with conclusions
"cancelled" and "timed_out" are routed the same as failures: use
GetMockWorkflowRunEvent(actionCompleted, "cancelled", ...) and
GetMockWorkflowRunEvent(actionCompleted, "timed_out", ...), set up mockKvStore
to return a subscription that includes Features(featureWorkflowRunFailure) (same
pattern as the existing "successful workflow run failure notification" case),
and expect mockAPI.CreatePost to be called once; ensure the test names are
unique (e.g., "workflow run cancelled notification" and "workflow run timed_out
notification") so TestPostWorkflowRunEvent covers these cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ca5ff94f-827b-4cb9-ad5d-35b682bc82ed
📒 Files selected for processing (4)
server/plugin/template.goserver/plugin/test_utils.goserver/plugin/webhook.goserver/plugin/webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/plugin/webhook.go
marianunez
approved these changes
Mar 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds support to workflow run success and failure messages under the
workflow_run_successandworkflow_run_failureeventsTicket Link
Fixes https://mattermost.atlassian.net/browse/MM-67473
Change Impact: 🟠 Medium
Reasoning: The PR adds new subscription features, templates, and webhook handling across multiple server modules (webhook, templates, subscriptions, commands), expanding notification surface without modifying auth or data persistence.
Regression Risk: Moderate — it changes routing and rendering logic for notifications and adds public template/feature flags; tests cover many cases (including success, failure, cancelled, timed_out), but integration edge cases (org-member filters, posting failures, template rendering variations) could surface regressions.
** QA Recommendation:** Perform focused manual QA on workflow-run notifications: subscribe/unsubscribe flows for both success and failure, verify org-member exclusion/inclusion behavior, and validate message rendering for success, failure, cancelled, and timed_out conclusions; do not skip manual QA.
Generated by CodeRabbitAI
Release Notes