Create Latest Version Servers for Desktop and mobile e2e#87
Merged
yasserfaraazkhan merged 4 commits intomasterfrom Apr 15, 2026
Merged
Create Latest Version Servers for Desktop and mobile e2e#87yasserfaraazkhan merged 4 commits intomasterfrom
yasserfaraazkhan merged 4 commits intomasterfrom
Conversation
Contributor
yasserfaraazkhan
commented
Apr 15, 2026
yasserfaraazkhan
commented
mm-cloud-bot
added
the
release-note-none
There was a problem hiding this comment.
Pull request overview
Updates Matterwick E2E provisioning to support a configurable "latest" server version by resolving the newest stable Mattermost release tag, and propagates the resolved version through desktop/mobile workflows and CMT runs.
Changes:
- Add
resolveE2EServerVersion()and switch E2E/CMT provisioning paths to use it (including normalizingv-prefixed versions for Docker tags). - Stop canceling in-progress PR workflow runs when the E2E label is removed (to avoid killing runs that remove the label on completion).
- Update defaults/tests to expect resolved server versions (and add GitHub releases API mocks for
"latest"resolution).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
server/workflow_run.go |
Uses resolved server version for nightly provisioning; strips v prefix for CMT server versions. |
server/server.go |
Adds githubAPIBase override for redirecting GitHub API calls in tests. |
server/push_events.go |
Uses resolved server version for push-event instance provisioning when no explicit version is provided. |
server/pull_request.go |
Changes behavior on E2E label removal to keep workflow runs alive. |
server/e2e_tests.go |
Implements "latest" resolution via GitHub releases API; propagates resolved server version into provisioning/dispatch. |
server/e2e_dryrun_test.go |
Adds/updates dry-run tests for resolved server version behavior and GitHub releases API mocking. |
config/config-matterwick.default.json |
Changes default E2EServerVersion from "master" to "latest". |
Comments suppressed due to low confidence (1)
server/e2e_tests.go:996
- dispatchDesktopE2EWorkflow() always calls resolveE2EServerVersion() before attempting to read server_version from instanceDetailsJSON. When instanceDetailsJSON is present (the common case), this does an unnecessary GitHub API call. Consider parsing instanceDetailsJSON first and only falling back to resolveE2EServerVersion() when the instance details are missing/empty.
// Determine the server version to use for the workflow.
// Default to the configured E2E server version, but prefer the actual
// provisioned version from the instance details when available.
serverVersion := s.resolveE2EServerVersion()
if instanceDetailsJSON != "" {
var instances []struct {
ServerVersion string `json:"server_version"`
}
if err := json.Unmarshal([]byte(instanceDetailsJSON), &instances); err == nil {
if len(instances) > 0 && instances[0].ServerVersion != "" {
serverVersion = instances[0].ServerVersion
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds dynamic resolution and short-lived caching for the E2E Mattermost server version when config is "latest"; propagates resolved versions into instance creation and workflow dispatching; normalizes leading Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant GitHubAPI
participant CloudInst
participant Workflow
Client->>Server: Request E2E provisioning (E2EServerVersion="latest")
activate Server
Server->>GitHubAPI: GET /repos/mattermost/mattermost/releases
GitHubAPI-->>Server: Releases list
Note over Server: Filter drafts/prereleases and tags with -rc/-beta/-alpha\nStrip leading 'v', select first stable tag or fallback "master"\nCache result (1 hour)
Server->>CloudInst: Create instance with resolved ServerVersion
CloudInst-->>Server: Instance created (ServerVersion)
Server->>Workflow: Dispatch workflow with MM_SERVER_VERSION = instance.ServerVersion
Workflow-->>Server: Workflow triggered
deactivate Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/e2e_tests.go (1)
983-996: Avoid a secondlatestlookup wheninstance_detailsalready has the provisioned version.Lines 986-996 resolve the server version before inspecting
instance_details, even though the desktop push/nightly paths already serialise the exact provisioned version into that JSON. That adds an unnecessary outbound GitHub call on every dispatch and spends token budget for no gain.Proposed diff
- serverVersion := s.resolveE2EServerVersion() + serverVersion := "" if instanceDetailsJSON != "" { var instances []struct { ServerVersion string `json:"server_version"` } if err := json.Unmarshal([]byte(instanceDetailsJSON), &instances); err == nil { if len(instances) > 0 && instances[0].ServerVersion != "" { serverVersion = instances[0].ServerVersion } } } + if serverVersion == "" { + serverVersion = s.resolveE2EServerVersion() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/e2e_tests.go` around lines 983 - 996, The code calls s.resolveE2EServerVersion() before inspecting instanceDetailsJSON, causing an unnecessary external "latest" lookup when the provisioned version is already encoded in instanceDetailsJSON; change the logic in the block around serverVersion/instanceDetailsJSON so you first parse instanceDetailsJSON (unmarshal into the local instances struct) and only call s.resolveE2EServerVersion() if no provioned ServerVersion is present, i.e., set serverVersion from instances[0].ServerVersion when available and fallback to s.resolveE2EServerVersion() only otherwise.server/push_events.go (1)
157-160: Short-circuit the explicit branch version before resolvinglatest.Lines 157-160 call
resolveE2EServerVersion()even whenversionis already known from the release branch. With"latest"now being the default, every release-branch push pays an avoidable GitHub API call and spends token budget for no change in behaviour.Proposed diff
- serverVersion := s.resolveE2EServerVersion() - if version != "" { - serverVersion = version - } + serverVersion := version + if serverVersion == "" { + serverVersion = s.resolveE2EServerVersion() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/push_events.go` around lines 157 - 160, Currently resolveE2EServerVersion() is always called before applying an explicit branch `version`, wasting API calls; change the logic so you use the provided `version` if non-empty and only call s.resolveE2EServerVersion() when `version` is empty. Update the code around the `serverVersion` assignment (the `serverVersion` local and the `version` variable) to short-circuit and avoid calling resolveE2EServerVersion() when `version` is already set.
🤖 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/e2e_tests.go`:
- Around line 739-765: The releases struct and loop currently only check Draft
and tag name patterns but miss GitHub's explicit prerelease flag; add a
Prerelease bool field to the anonymous releases struct and in the for loop skip
any release where r.Prerelease is true (in the same place where you skip r.Draft
and the "-rc/-beta/-alpha" patterns) so the function (resolveE2EServerVersion)
will not return a prerelease version; keep the rest of the logic
(strings.ToLower, strings.TrimPrefix, s.Logger.WithField(...).Info(...), return
version) unchanged.
---
Nitpick comments:
In `@server/e2e_tests.go`:
- Around line 983-996: The code calls s.resolveE2EServerVersion() before
inspecting instanceDetailsJSON, causing an unnecessary external "latest" lookup
when the provisioned version is already encoded in instanceDetailsJSON; change
the logic in the block around serverVersion/instanceDetailsJSON so you first
parse instanceDetailsJSON (unmarshal into the local instances struct) and only
call s.resolveE2EServerVersion() if no provioned ServerVersion is present, i.e.,
set serverVersion from instances[0].ServerVersion when available and fallback to
s.resolveE2EServerVersion() only otherwise.
In `@server/push_events.go`:
- Around line 157-160: Currently resolveE2EServerVersion() is always called
before applying an explicit branch `version`, wasting API calls; change the
logic so you use the provided `version` if non-empty and only call
s.resolveE2EServerVersion() when `version` is empty. Update the code around the
`serverVersion` assignment (the `serverVersion` local and the `version`
variable) to short-circuit and avoid calling resolveE2EServerVersion() when
`version` is already set.
🪄 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: c49f6326-af7e-4273-a607-58d322b51b65
📒 Files selected for processing (7)
config/config-matterwick.default.jsonserver/e2e_dryrun_test.goserver/e2e_tests.goserver/pull_request.goserver/push_events.goserver/server.goserver/workflow_run.go
…rVersion - Add `prerelease` boolean field check (GitHub's explicit flag) alongside the existing draft and tag-name pattern filters so releases marked prerelease on GitHub are never selected even when their tag looks stable. - Replace context.Background() with a 10-second timeout to prevent instance-creation goroutines from blocking indefinitely on slow/unavailable GitHub API. - Cache the resolved version in-memory for 1 hour so back-to-back provisioning calls share one API round-trip. Fallback "master" results are intentionally not cached so the next call retries the API. - Add test coverage for all three new behaviours. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/e2e_tests.go (1)
480-503:⚠️ Potential issue | 🟡 MinorGuard the desktop path against an empty
instancesslice.
instances[0].ServerVersionwill panic if this method is ever called with no instances. The mobile path already fails closed on bad cardinality; desktop should do the same.Suggested fix
func (s *Server) triggerDesktopE2EWorkflow(ctx context.Context, client *github.Client, pr *model.PullRequest, instances []*E2EInstance) error { + if len(instances) == 0 { + return fmt.Errorf("desktop E2E requires at least 1 instance") + } + logger := s.Logger.WithFields(logrus.Fields{ "repo": pr.RepoName, "pr": pr.Number, "type": "desktop",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/e2e_tests.go` around lines 480 - 503, triggerDesktopE2EWorkflow can panic when instances is empty because it accesses instances[0].ServerVersion; add a guard at the start of triggerDesktopE2EWorkflow that checks len(instances) == 0 and returns a descriptive error (matching the mobile path behavior) before calling buildInstanceDetailsJSON or referencing instances[0]; ensure any downstream logic that expects at least one instance is skipped when the check fails.
🧹 Nitpick comments (1)
server/e2e_dryrun_test.go (1)
185-197: Please drive these assertions through the real dispatch/CMT code paths.These subtests rebuild the payload or reapply the normalisation/cap logic inline, so they can keep passing even if the production methods stop doing it. Using
mockGitHubServerplus the real dispatch/CMT entrypoints would give this change much stronger regression coverage.Also applies to: 1068-1097, 1167-1198, 1200-1218, 1251-1259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/e2e_dryrun_test.go` around lines 185 - 197, Instead of reconstructing workflowInputs inline, drive the test through the real code path by using mockGitHubServer to send the same webhook/event that hits the actual dispatch/CMT entrypoint so the production payload normalization and cap logic run; replace the inline map named workflowInputs with a call that triggers the dispatch/CMT handler and capture the built payload, then assert the captured payload fields (e.g., MM_SERVER_VERSION equals instances[0].ServerVersion and MM_TEST_USER_NAME equals s.Config.E2EUsername). Apply the same change to the other subtests referenced (lines around 1068-1097, 1167-1198, 1200-1218, 1251-1259) so all assertions validate the real dispatch/CMT output rather than locally-rebuilt payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/e2e_tests.go`:
- Around line 480-503: triggerDesktopE2EWorkflow can panic when instances is
empty because it accesses instances[0].ServerVersion; add a guard at the start
of triggerDesktopE2EWorkflow that checks len(instances) == 0 and returns a
descriptive error (matching the mobile path behavior) before calling
buildInstanceDetailsJSON or referencing instances[0]; ensure any downstream
logic that expects at least one instance is skipped when the check fails.
---
Nitpick comments:
In `@server/e2e_dryrun_test.go`:
- Around line 185-197: Instead of reconstructing workflowInputs inline, drive
the test through the real code path by using mockGitHubServer to send the same
webhook/event that hits the actual dispatch/CMT entrypoint so the production
payload normalization and cap logic run; replace the inline map named
workflowInputs with a call that triggers the dispatch/CMT handler and capture
the built payload, then assert the captured payload fields (e.g.,
MM_SERVER_VERSION equals instances[0].ServerVersion and MM_TEST_USER_NAME equals
s.Config.E2EUsername). Apply the same change to the other subtests referenced
(lines around 1068-1097, 1167-1198, 1200-1218, 1251-1259) so all assertions
validate the real dispatch/CMT output rather than locally-rebuilt payloads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d74676fb-deeb-49af-a960-8130b69da9e8
📒 Files selected for processing (3)
server/e2e_dryrun_test.goserver/e2e_tests.goserver/server.go
✅ Files skipped from review due to trivial changes (1)
- server/server.go
Replace inline hand-built payload maps in three tests with calls to the actual triggerDesktopE2EWorkflow / triggerMobileE2EWorkflow functions against a mock GitHub server, so assertions validate real dispatch output rather than maps the tests constructed themselves. Add newTestGitHubClient helper that creates a *github.Client whose BaseURL points at a given httptest.Server. CMT instance-construction tests are left as-is: handleCMTWithServerVersions requires a CloudClient that the intentionally-lightweight dry-run suite does not provide. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/e2e_dryrun_test.go (1)
51-103:⚠️ Potential issue | 🟡 MinorAdd
t.Cleanup(srv.Close)to prevent test resource leaks.
mockGitHubServerdoesn't register cleanup for the returnedhttptest.Server, whilemockReleasesServer(line 878) does. This inconsistency leads to potential resource leaks whenmockGitHubServeris used (lines 203, 1113, 1149).🧹 Proposed fix
func mockGitHubServer(t *testing.T, status int) (*httptest.Server, *[]dispatchCapture) { t.Helper() var mu sync.Mutex var captures []dispatchCapture srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // ... handler code ... })) + t.Cleanup(srv.Close) return srv, &captures }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/e2e_dryrun_test.go` around lines 51 - 103, The mockGitHubServer helper creates an httptest.Server (srv) but never registers t.Cleanup, which can leak resources; inside mockGitHubServer, immediately after creating srv (the variable named srv returned by the function), call t.Cleanup(srv.Close) so the test harness will close the httptest.Server automatically; keep returning (srv, &captures) unchanged and ensure this is done in the mockGitHubServer function before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/e2e_dryrun_test.go`:
- Around line 51-103: The mockGitHubServer helper creates an httptest.Server
(srv) but never registers t.Cleanup, which can leak resources; inside
mockGitHubServer, immediately after creating srv (the variable named srv
returned by the function), call t.Cleanup(srv.Close) so the test harness will
close the httptest.Server automatically; keep returning (srv, &captures)
unchanged and ensure this is done in the mockGitHubServer function before
returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8d86b2c-811f-48fb-bfcb-ce786ee42ee2
📒 Files selected for processing (1)
server/e2e_dryrun_test.go
mockGitHubServer was leaking the httptest.Server on test completion. Add t.Cleanup(srv.Close) immediately after server creation, consistent with the pattern already used in mockReleasesServer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
saturninoabril
approved these changes
Apr 15, 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
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.