fix(e2e): Fix push/release E2E trigger bugs and silent failures#89
Merged
yasserfaraazkhan merged 6 commits intomasterfrom May 6, 2026
Merged
fix(e2e): Fix push/release E2E trigger bugs and silent failures#89yasserfaraazkhan merged 6 commits intomasterfrom
yasserfaraazkhan merged 6 commits intomasterfrom
Conversation
Four bugs prevented E2E tests from triggering correctly on release
branches and master/main pushes:
**Bug A — Wrong server version for push events**
Release branch name ("release-9.0") was used to derive a Docker tag
("9.0"), but Cloud only accepts full SemVer tags like "9.0.0". This
caused installation create errors and the workflow was never dispatched.
Fix: `serverVersionForPushEvent` always calls `resolveE2EServerVersion()`
(returns latest stable) and ignores the branch name.
**Bug B — Empty E2EReleasePatternPrefix matches every branch**
`strings.HasPrefix(x, "")` is always true, so when the config field was
unset every branch (including master, feature branches) was misclassified
as a release branch. Fix: guard returns false on empty prefix.
**Bug C — Silent debug logs hide why events are dropped**
"Ignoring workflow_run event" and "Push event does not match E2E trigger
conditions" were Debug-level with no field values. Promoted to Info and
added `configured_nightly_name`, `configured_test_workflows`,
`auto_release`, `auto_master`, `release_pattern_prefix`, and
`is_release_branch` fields so operators can diagnose without enabling
debug logging.
**Bug D — Empty E2EServerVersion passed raw to Cloud API**
`resolveE2EServerVersion` returned `""` when `E2EServerVersion` was
unset, which the Cloud API rejected. Fix: trim whitespace, treat empty
as "latest" with a Warn log.
Also:
- Removed dead `extractVersionFromReleaseBranch` function and its test
- Added regression tests for all four bugs in e2e_dryrun_test.go
- Updated config/config-matterwick.default.json with missing fields
(E2EResetServersLabel, E2EPassword, E2EInstanceMaxAge)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@yasserfaraazkhan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements trimmed-value and "latest" fallback for E2E server-version resolution with a 1‑hour in-memory cache and GitHub Releases lookup; changes push-event E2E flow to always provision the latest stable release (removing per-branch version gating); adds E2E config fields and expands tests for empty/whitespace config and push-event behavior. ChangesE2E Push Event Flow & Version Resolution
Sequence DiagramsequenceDiagram
participant Git as GitHub Push
participant MW as Matterwick Server
participant Cache as Version Cache (1h TTL)
participant GH as GitHub Releases API
participant E2E as E2E Provisioner
Git->>MW: webhook (push event)
MW->>MW: isReleaseBranch(branch) (guard empty prefix)
MW->>MW: handlePushEventE2E(event, branch)
MW->>Cache: resolveE2EServerVersion(config)
alt Cache Hit
Cache-->>MW: cached latest version
else Cache Miss
MW->>GH: fetch latest stable release tag
GH-->>MW: latest stable version
MW->>Cache: store version + timestamp
Cache-->>MW: latest version
end
MW->>E2E: createMultipleE2EInstancesForPushEvent(repo, type, branch)
E2E->>E2E: derive serverVersion internally and select runner/platform
E2E-->>MW: instances created and dispatched
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/push_events.go (1)
170-170: 💤 Low valueConsider naming the unused parameter explicitly.
The blank identifier
_for the fourth parameter works, but Go convention often favours naming unused parameters to document their purpose (e.g.,_ string→sha stringwith_ = sha), consistent with howserverVersionForPushEventhandles its unusedbranchparameter on line 75.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/push_events.go` at line 170, The fourth parameter of createMultipleE2EInstancesForPushEvent is declared as the blank identifier; rename it to a descriptive name (e.g., sha string) and explicitly mark it unused in the function body (for example with _ = sha) to document its intent and mirror the pattern used in serverVersionForPushEvent; update any call sites if needed to pass the same positional argument name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/push_events.go`:
- Line 170: The fourth parameter of createMultipleE2EInstancesForPushEvent is
declared as the blank identifier; rename it to a descriptive name (e.g., sha
string) and explicitly mark it unused in the function body (for example with _ =
sha) to document its intent and mirror the pattern used in
serverVersionForPushEvent; update any call sites if needed to pass the same
positional argument name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14cf2647-11c6-4b4e-acaa-2bbd1eb895e0
📒 Files selected for processing (5)
config/config-matterwick.default.jsonserver/e2e_dryrun_test.goserver/e2e_tests.goserver/push_events.goserver/workflow_run.go
The fourth parameter was declared as the blank identifier `_`. Rename it to `sha string` and suppress it with `_ = sha` in the body, mirroring the pattern used in serverVersionForPushEvent, so the intent is documented at the declaration rather than being implicit. No behaviour change.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/push_events.go (1)
170-171: 💤 Low valueConsider removing unused
shaparameter.The
shaparameter is accepted but immediately suppressed. If it's not needed for version resolution (which now usesserverVersionForPushEvent(branch)), consider removing it from the signature to avoid confusion. The tracking key that usesshais constructed in the caller (handlePushEventE2E), not here.However, if this is intentionally kept for future use or API consistency, the current suppression with comment is acceptable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/push_events.go` around lines 170 - 171, The createMultipleE2EInstancesForPushEvent function currently accepts an unused parameter sha which is suppressed; remove sha from the function signature and all callers (e.g., handlePushEventE2E) and update any call sites and tests that pass the sha so tracking keys remain constructed where they currently are, or if you intend to keep sha for API consistency, explicitly document its intended future use and remove the blank suppression line; ensure compile by updating references to the function name createMultipleE2EInstancesForPushEvent and any call sites that relied on the old signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/push_events.go`:
- Around line 170-171: The createMultipleE2EInstancesForPushEvent function
currently accepts an unused parameter sha which is suppressed; remove sha from
the function signature and all callers (e.g., handlePushEventE2E) and update any
call sites and tests that pass the sha so tracking keys remain constructed where
they currently are, or if you intend to keep sha for API consistency, explicitly
document its intended future use and remove the blank suppression line; ensure
compile by updating references to the function name
createMultipleE2EInstancesForPushEvent and any call sites that relied on the old
signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31287ef5-df52-4618-8cab-e56b3acedd5d
📒 Files selected for processing (1)
server/push_events.go
…ushEvent sha was accepted but immediately suppressed with _ = sha. The tracking key and dispatch calls that use sha live in the caller (handlePushEventE2E), not here. Removing the parameter eliminates the confusion without any behaviour change.
mm-cloud-bot
added
the
release-note-none
yasserfaraazkhan
removed
do-not-merge/release-note-label-needed
release-note-none
mm-cloud-bot
added
release-note-none
Remove or shorten over-explained inline comments and docstrings across push_events.go, workflow_run.go, and e2e_tests.go. Keep comments that explain non-obvious intent (race condition guard, empty-prefix guard, RC tag secondary check, test mock redirect). Remove comments that restate what the code already shows.
andrleite
approved these changes
May 6, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/push_events.go (1)
221-233: 💤 Low valuePotential inconsistency in macOS runner label.
This helper uses
"macos-latest"for macOS, butbuildDesktopCMTMatrixJSONinworkflow_run.go(line 441) uses"macos-13". If both should target the same runner, consider aligning them. If the difference is intentional (e.g., CMT needs a specific macOS version while push events can use latest), a comment explaining the distinction would be helpful.#!/bin/bash # Description: Check for all macos runner references to verify consistency rg -n "macos" --type go -C2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/push_events.go` around lines 221 - 233, getRunnerForPlatform currently returns "macos-latest" while buildDesktopCMTMatrixJSON uses "macos-13", causing potential mismatch; either make them consistent by returning "macos-13" from getRunnerForPlatform (update the switch case for "macos") if both should target the same runner, or keep "macos-latest" but add a concise comment above getRunnerForPlatform explaining why buildDesktopCMTMatrixJSON intentionally pins "macos-13" (e.g., CMT requires macOS 13), and if choosing the latter, add a unit-test or linter note referencing buildDesktopCMTMatrixJSON to prevent future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/push_events.go`:
- Around line 221-233: getRunnerForPlatform currently returns "macos-latest"
while buildDesktopCMTMatrixJSON uses "macos-13", causing potential mismatch;
either make them consistent by returning "macos-13" from getRunnerForPlatform
(update the switch case for "macos") if both should target the same runner, or
keep "macos-latest" but add a concise comment above getRunnerForPlatform
explaining why buildDesktopCMTMatrixJSON intentionally pins "macos-13" (e.g.,
CMT requires macOS 13), and if choosing the latter, add a unit-test or linter
note referencing buildDesktopCMTMatrixJSON to prevent future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 579c6e5d-f37f-4cb3-a829-a6f1057cc305
📒 Files selected for processing (3)
server/e2e_tests.goserver/push_events.goserver/workflow_run.go
getRunnerForPlatform uses macos-latest (E2E functional workflows track latest) while buildDesktopCMTMatrixJSON pins macos-13 (CMT requires a specific OS version for compatibility testing). Add a comment to make the distinction explicit and prevent future confusion.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/push_events.go`:
- Around line 19-21: Before converting refs to branch names, ensure you ignore
non-branch refs by checking event.GetRef() (branchRef) starts with
"refs/heads/"; if it doesn't, skip/return early to avoid treating tags
(refs/tags/*) as branches. Update the logic around branchRef := event.GetRef()
and the later branch name evaluation (where extractBranchName is called and
where branch-trigger logic runs, including the code block around lines handling
branch matching at 29-33) so only "refs/heads/*" are processed for
branch-trigger evaluation.
- Around line 58-63: The comment for serverVersionForPushEvent is misleading
about behavior; update the function comment to state that the server version is
resolved via resolveE2EServerVersion (and may include its fallback logic) rather
than asserting it "always returns the latest stable release." Reference the
function name serverVersionForPushEvent and the resolver resolveE2EServerVersion
in the new comment so future readers know where to look for actual resolution
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24343760-2500-4a02-a323-aea66f8ac0cc
📒 Files selected for processing (1)
server/push_events.go
Tag pushes (refs/tags/*) were passed through extractBranchName and could accidentally match isReleaseBranch — e.g. "refs/tags/release-9.0" extracts to "release-9.0" which matches the "release-" prefix. Added an early return for any ref that does not start with "refs/heads/". Also correct the serverVersionForPushEvent comment: resolveE2EServerVersion can fall back to "master" on API error, so "always returns latest stable" was inaccurate.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/e2e_dryrun_test.go (1)
448-458: ⚡ Quick winMake the tag-ref regression test behaviour-driven at
handlePushEvent.Right now this subtest validates assumptions about strings/parsing, but not the actual early-return branch in production flow. Consider asserting that
handlePushEventlogs the non-branch skip path forrefs/tags/*.Proposed test hardening
+import "bytes" @@ t.Run("tag refs are not treated as branch refs", func(t *testing.T) { @@ assert.Equal(t, "release-9.0", extractBranchName(tagRef), "extractBranchName is unaware of ref type; caller must pre-filter") }) + + t.Run("handlePushEvent skips tag refs before branch extraction", func(t *testing.T) { + s := newDryRunServer(t, "", "mattermost") + s.Config.E2EAutoTriggerOnRelease = true + s.Config.E2EAutoTriggerOnMaster = true + + buf := &bytes.Buffer{} + logger := logrus.New() + logger.SetOutput(buf) + s.Logger = logger + + event := &gogithub.PushEvent{ + Ref: gogithub.String("refs/tags/release-9.0"), + Repo: &gogithub.PushEventRepository{ + Name: gogithub.String("mattermost-desktop"), + }, + } + + s.handlePushEvent(event) + assert.Contains(t, buf.String(), "Push ref is not a branch, skipping E2E trigger") + assert.NotContains(t, buf.String(), "Push event received") + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/e2e_dryrun_test.go` around lines 448 - 458, The test currently only asserts string parsing; update it to exercise the production path by calling handlePushEvent with a push payload whose ref is "refs/tags/release-9.0" and assert the function takes the non-branch early-return path (e.g., verify the mock logger recorded the "skipping non-refs/heads" or equivalent message and that no provisioning functions were invoked). Keep extractBranchName assertions if you want, but the key is to call handlePushEvent and assert it does not treat tag refs as branch refs by checking the logger and that downstream actions (provisioning/mocked handlers) were not called.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/e2e_dryrun_test.go`:
- Around line 448-458: The test currently only asserts string parsing; update it
to exercise the production path by calling handlePushEvent with a push payload
whose ref is "refs/tags/release-9.0" and assert the function takes the
non-branch early-return path (e.g., verify the mock logger recorded the
"skipping non-refs/heads" or equivalent message and that no provisioning
functions were invoked). Keep extractBranchName assertions if you want, but the
key is to call handlePushEvent and assert it does not treat tag refs as branch
refs by checking the logger and that downstream actions (provisioning/mocked
handlers) were not called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29373ecf-3349-49fa-a3e5-bc0e5ad6765a
📒 Files selected for processing (2)
server/e2e_dryrun_test.goserver/push_events.go
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
3 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
Fixes four independent defects that prevented E2E workflows from triggering reliably on
release/*branches andmain/masterpushes. The changes harden version resolution, correct branch classification, and improve observability for dropped events.Root Causes & Fixes
1) Incorrect server version derived from push events
Problem
For release branches (e.g.,
release-9.0), the branch name was converted into a Docker tag (9.0). The Cloud API requires full SemVer (e.g.,9.0.0), causing installation creation to fail and the workflow to never dispatch.Fix
serverVersionForPushEventnow always callsresolveE2EServerVersion()(which returns the latest stable release) and intentionally ignores the branch name.2) Empty
E2EReleasePatternPrefixmatches all branchesProblem
strings.HasPrefix(x, "")is alwaystrue. WhenE2EReleasePatternPrefixwas unset, every branch (includingmainand feature branches) was misclassified as a release branch.Fix
isReleaseBranchreturnsfalseimmediately when the prefix is empty.3) Insufficient logging for dropped events
Problem
Critical drop-path logs (e.g., “Ignoring workflow_run event”, “Push event does not match E2E trigger conditions”) were at
Debuglevel without contextual fields, making diagnosis impractical without debug logging.Fix
Promoted logs to
Infoand added diagnostic fields:configured_nightly_nameconfigured_test_workflowsauto_releaseauto_masterrelease_pattern_prefixis_release_branch4) Empty
E2EServerVersionpassed to Cloud APIProblem
resolveE2EServerVersionreturned""whenE2EServerVersionwas unset; the Cloud API rejects empty versions.Fix
Trim whitespace and treat empty as
"latest", with aWarnlog.Impact
main/masterand release branches.Notes
Also included
extractVersionFromReleaseBranchfunction and its testserver/e2e_dryrun_test.goconfig/config-matterwick.default.jsonwith missing fields (E2EResetServersLabel,E2EPassword,E2EInstanceMaxAge)Release Note
🤖 Generated with Claude Code