Update dependencies and recover build tooling from starter template#1
Conversation
- server/public v0.1.10 -> v0.4.3 (requires go 1.26.3) - jellydator/ttlcache/v3 v3.4.0 -> v3.4.1 - lmittmann/tint v1.1.2 -> v1.1.3 - stretchr/testify v1.10.0 -> v1.11.1 - mellium.im/xmpp v0.22.0 -> v0.23.0
Replace the custom .mk build system (which shelled out to a separately-installed global pluginctl binary) with the starter template's self-contained build/manifest and build/pluginctl Go tools, compiled in-repo via build/setup.mk. This removes the dependency on an externally installed pluginctl and its version gate against plugin.json's props.pluginctl field. - Restore build/manifest and build/pluginctl from the starter template (compiled to ./build/bin by the Makefile) - Replace the modular build/*.mk files with the template's Makefile; XMPP dev-server and mock targets preserved in build/custom.mk - Drop go.mod's vestigial `tool` directive (golangci-lint v1.61.0), since golangci-lint is now installed via `go install` like the template - Update .nvmrc to match the template - Adopt template's ci.yml (id-token permission, pinned workflow ref), keeping the `main` branch trigger - Temporarily exclude build/manifest and build/pluginctl from lint (config swap to the template's golangci config follows separately)
Replace .golangci.yml with the template's config (bidichk, makezero,
modernize, unqueryvet, govet enable-all, gofumpt) now that
build/manifest and build/pluginctl no longer need a lint exclusion
tailored to a different config. Fix the resulting findings:
- Check previously-ignored error returns (Body.Close, conn.Close,
encoder.Flush) explicitly
- Rename shadowed `err` variables in cmd/xmpp-client-doctor
- Use sync.WaitGroup.Go instead of manual Add/Done pairs
- Use maps.Copy instead of a manual copy loop
- Replace interface{} with any; gofumpt formatting
`make mock` failed for two independent reasons: - The Command interface declared an unexported method (executeXMPPBridgeCommand) that is only ever called on the concrete *Handler from within Handle() and never through the interface. Go only allows unexported interface methods to be implemented by types in the same package, so any mock generated into the separate `mocks` package could never satisfy Command. Removed it from the interface; behavior is unchanged since it was never invoked via the interface value. - mockgen's reflect mode needs to resolve this module by import path, which fails for a local, unpushed module. Switched the mock target to -source mode, which reads server/command/command.go directly. Also add go.uber.org/mock as a direct dependency and run `go mod tidy`, which additionally drops golangci-lint v1.61.0/gotestsum's transitive dependency tree from go.mod/go.sum — orphaned since the `tool` directive was removed in a previous commit but not yet tidied away.
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR rewrites the build and release tooling around a new Makefile, setup/custom includes, a Go manifest generator, and a pluginctl CLI. It also updates CI, lint, Node, and dependency configuration, plus small Go cleanups in server, XMPP, and command code. ChangesBuild System Restructuring
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
build/pluginctl/main.go (1)
69-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant shadowed
clientin the username/password branch.Line 94 already assigns the outer
clienttomodel.NewAPIv4Client(siteURL). Theclient := model.NewAPIv4Client(siteURL)at line 103 shadows it with a duplicate, equivalent instance instead of reusing the one already created — unnecessary object creation and a shadowed variable the PR's own lint cleanup theme (shadowed variables) targets elsewhere.♻️ Suggested fix
if adminUsername != "" && adminPassword != "" { - client := model.NewAPIv4Client(siteURL) log.Printf("Authenticating as %s against %s.", adminUsername, siteURL) _, _, err := client.Login(ctx, adminUsername, adminPassword) if err != nil { return nil, fmt.Errorf("failed to login as %s: %w", adminUsername, err) } return client, nil }🤖 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 `@build/pluginctl/main.go` around lines 69 - 114, The username/password branch in getClient is creating a second shadowed client instead of reusing the one already initialized from model.NewAPIv4Client(siteURL). Remove the inner client declaration in the adminUsername/adminPassword path and use the existing client variable for Login so the function avoids unnecessary duplication and eliminates the shadowed variable.go.mod (1)
3-3: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winGo patch version is one release behind.
go 1.26.3is valid, butgo1.26.4(released 2026-06-02) is now out with additional security fixes tocrypto/x509,mime, andnet/textproto. Worth bumping alongside the other dependency refresh in this PR.🤖 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 `@go.mod` at line 3, The Go toolchain version in go.mod is one patch release behind, so update the module’s go directive from the current 1.26.3 to the latest 1.26.4 as part of this dependency refresh. Keep the change limited to the version declaration in go.mod and ensure the project still builds with the updated Go release.
🤖 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 @.github/workflows/ci.yml:
- Around line 14-19: The reusable workflow call in the plugin-ci job is tracking
a mutable upstream ref, which should be pinned to a specific release tag or
commit SHA instead of `@main`. Update the uses reference for
mattermost/actions-workflows/.github/workflows/plugin-ci.yml to a fixed,
auditable ref while keeping the existing plugin-ci job and secrets: inherit
configuration intact.
In `@build/pluginctl/main.go`:
- Around line 18-24: The helpText usage block is incomplete because it omits the
logs and logs-watch subcommands that pluginctl() already supports. Update the
helpText constant in main.go to include documentation for those two commands
alongside deploy, disable, enable, and reset so the printed usage matches the
available command handling.
---
Nitpick comments:
In `@build/pluginctl/main.go`:
- Around line 69-114: The username/password branch in getClient is creating a
second shadowed client instead of reusing the one already initialized from
model.NewAPIv4Client(siteURL). Remove the inner client declaration in the
adminUsername/adminPassword path and use the existing client variable for Login
so the function avoids unnecessary duplication and eliminates the shadowed
variable.
In `@go.mod`:
- Line 3: The Go toolchain version in go.mod is one patch release behind, so
update the module’s go directive from the current 1.26.3 to the latest 1.26.4 as
part of this dependency refresh. Keep the change limited to the version
declaration in go.mod and ensure the project still builds with the updated Go
release.
🪄 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: 72262678-3926-4031-9405-696e1dbdd3fa
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
.github/workflows/ci.yml.golangci.yml.nvmrcMakefilebuild/build.mkbuild/custom.mkbuild/custom_xmpp_server.mkbuild/deploy.mkbuild/dev.mkbuild/manifest/main.gobuild/pluginctl/logs.gobuild/pluginctl/logs_test.gobuild/pluginctl/main.gobuild/setup.mkbuild/test.mkbuild/utils.mkbuild/versioning.mkcmd/xmpp-client-doctor/main.gogo.modplugin.jsonserver/bridge/manager.goserver/bridge/mattermost/bridge.goserver/bridge/mattermost/message_handler.goserver/bridge/messagebus.goserver/command/command.goserver/command/mocks/mock_commands.goserver/config/config.goserver/configuration.goserver/plugin_test.goserver/xmpp/client.goserver/xmpp/xep_0077.go
💤 Files with no reviewable changes (9)
- server/command/command.go
- build/custom_xmpp_server.mk
- build/versioning.mk
- build/dev.mk
- build/test.mk
- build/utils.mk
- build/build.mk
- build/deploy.mk
- plugin.json
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
server/publicv0.1.10 → v0.4.3 (and 4 other direct deps), bumping thegodirective to 1.26.3.mattermost-plugin-starter-template: replace the custom.mkbuild system, which shelled out to a separately-installed globalpluginctlbinary, with the template's self-containedbuild/manifest+build/pluginctlGo tools compiled in-repo. No externalpluginctlinstall is required anymore. XMPP dev-server andmocktargets are preserved inbuild/custom.mk..golangci.yml(bidichk, makezero, modernize, govet enable-all, gofumpt) and fix the resulting findings (unchecked error returns, shadowed variables,sync.WaitGroup.Go,maps.Copy,interface{}→any).make mock, which was broken for two independent reasons: theCommandinterface declared an unexported method that Go only allows same-package types to implement (so no cross-package mock could ever satisfy it — removed since it was never invoked through the interface anyway), andmockgen's reflect mode can't resolve this module locally (switched to-sourcemode). This also letgo mod tidydrop the golangci-lint v1.61.0/gotestsum dependency tree that had been orphaned since thetooldirective was dropped.Verified end-to-end:
make all(check-style → test → dist) passes with 0 lint issues and all tests green; all othermaketargets not requiring a live Mattermost server, Docker daemon, or destructive git push were also exercised successfully.Ticket Link
N/A