Add proper mqtt2http version in the info API endpoint - #24
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses issue #10 by adding proper mqtt2http version information to the info API endpoint. Previously, the root endpoint (/) returned only the underlying MQTT broker's version (from the mochi-mqtt library), which was confusing for users trying to identify the mqtt2http version.
Changes:
- Introduced version injection at build time via Docker build args and Go ldflags
- Modified API response structure to include both mqtt2http version and broker information
- Added version logging at startup for better visibility
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmd/mqtt2http.go | Added version variable with ldflags injection and default "dev" fallback |
| broker/config.go | Extended BrokerConfig to include Version field |
| broker/broker.go | Added version logging at startup and passes version to API controller |
| api/controller.go | Restructured root endpoint response to wrap broker info with mqtt2http version in AppInfo struct |
| Dockerfile | Added BUILD_VERSION build arg and configured ldflags for version injection |
| .github/workflows/build.yml | Added BUILD_VERSION build arg to release workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| platforms: linux/amd64,linux/arm64,linux/arm/v7 | ||
| tags: ${{ steps.meta.outputs.tags }} | ||
| labels: ${{ steps.meta.outputs.labels }} | ||
| build-args: BUILD_VERSION=${{ github.ref_name }} |
There was a problem hiding this comment.
The build-args parameter should also be added to the build-latest job (around line 66-75 in the file) to ensure builds from the main branch don't always default to "dev" version. Without this, the latest Docker tag will show version="dev" instead of a meaningful version identifier. Consider passing BUILD_VERSION for consistency across all build jobs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type AppInfo struct { | ||
| Version string `json:"version"` | ||
| Broker *system.Info `json:"broker"` | ||
| } |
There was a problem hiding this comment.
The root endpoint response shape changes from returning the broker info directly to wrapping it under broker. That is a breaking API change for any consumers expecting the previous top-level fields. If the goal is only to fix the version field, consider preserving the existing JSON structure and overriding/adding fields (e.g., keep broker fields at the top-level and introduce a separate brokerVersion/mqttVersion field) rather than nesting everything under broker.
| Broker *system.Info `json:"broker"` | ||
| } | ||
|
|
||
| func NewController(version string, server *mqtt.Server, store *lib.ClientStore, password string) *Controller { |
There was a problem hiding this comment.
version can be an empty string (e.g., tests/other callers construct BrokerConfig without setting it), which would make the / endpoint report an empty version. Consider defaulting version to something like "dev" inside NewController (or earlier when building BrokerConfig) when an empty value is passed in.
| func NewController(version string, server *mqtt.Server, store *lib.ClientStore, password string) *Controller { | |
| func NewController(version string, server *mqtt.Server, store *lib.ClientStore, password string) *Controller { | |
| if version == "" { | |
| version = "dev" | |
| } |
| func (c *Controller) RootHandler() http.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| info, _ := json.Marshal(c.server.Info) | ||
| info := &AppInfo{ | ||
| Version: c.version, | ||
| Broker: c.server.Info.Clone(), | ||
| } | ||
|
|
||
| data, err := json.Marshal(info) | ||
| if err != nil { | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| io.WriteString(w, err.Error()) | ||
| return | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Write(info) | ||
| w.Write(data) | ||
| } |
There was a problem hiding this comment.
New behavior is introduced for / (including application version and broker info nesting), but there are no tests asserting the root endpoint’s JSON structure or version value. Adding a focused test would prevent regressions and clarify the intended contract for the info endpoint.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Closes #10