feat: add arm64 docker builds and better cross platform support#46
feat: add arm64 docker builds and better cross platform support#46thushan merged 9 commits intothushan:mainfrom
Conversation
WalkthroughThe changes introduce multi-architecture (amd64 and arm64) support to the Docker build and release process, update CI workflows to test builds across multiple platforms, and enhance documentation and makefile targets for cross-platform validation. The application’s version output and system summary now include version, commit, OS, and architecture details. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CI as GitHub Actions CI
participant Docker as Docker Buildx/QEMU
participant Registry as Docker Registry
Dev->>CI: Push code / trigger release
CI->>Docker: Setup QEMU (multi-arch emulation)
CI->>Docker: Setup Buildx for amd64 & arm64
CI->>Docker: Build Docker images (amd64, arm64)
Docker->>Registry: Push architecture-specific images
CI->>Docker: Create multi-arch manifest (latest, version tags)
Docker->>Registry: Push manifest (references both images)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.goreleaser.yml (1)
87-112: LGTM: arm64 images added with matching labels and extra filesArm64 block mirrors amd64 correctly and uses buildx with the proper platform flag. This satisfies the PR objective to add arm64 builds.
If you want to reduce duplication between the two docker blocks, consider YAML anchors for shared labels and extra_files.
.github/workflows/release.yml (1)
30-32: Good addition: QEMU setup enables cross-arch buildsThis is required for arm64 emulation on the GitHub runner and aligns with the goal of publishing arm64 images.
Optionally, restrict to just the platforms you need:
- name: Set up Docker QEMU uses: docker/setup-qemu-action@v3 + with: + platforms: arm64,amd64
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.yml(1 hunks).goreleaser.yml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-27T12:59:29.787Z
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.787Z
Learning: Go version 1.24 or higher must be used.
Applied to files:
.goreleaser.yml
🪛 YAMLlint (1.37.1)
.goreleaser.yml
[warning] 116-116: wrong indentation: expected 6 but found 8
(indentation)
[warning] 120-120: wrong indentation: expected 6 but found 8
(indentation)
[warning] 124-124: wrong indentation: expected 6 but found 8
(indentation)
[warning] 128-128: wrong indentation: expected 6 but found 8
(indentation)
🔇 Additional comments (2)
.goreleaser.yml (1)
63-67: LGTM: amd64 image tagging and buildx flags look correctThe amd64 block is consistent and aligns with multi-arch manifest creation. No issues spotted.
.github/workflows/release.yml (1)
9-9: Go version matches team requirementUsing GO_VERSION=1.24.x complies with the documented team learning that Go 1.24+ must be used.
| uses: docker/setup-buildx-action@v3 | ||
| with: | ||
| platforms: linux/amd64 | ||
| platforms: linux/amd64,linux/arm64 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Incorrect input: docker/setup-buildx-action does not support 'platforms'
The setup-buildx action doesn’t accept a 'platforms' input; this likely causes the workflow to error. Either remove it or move the platforms specification to the QEMU step (or to the build step when invoking build-push, which GoReleaser will handle internally).
Apply:
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3
- with:
- platforms: linux/amd64,linux/arm64Optionally, pin platforms on QEMU setup (see previous comment).
🌐 Web query:
Does docker/setup-buildx-action@v3 support an input named "platforms", or should platforms be configured via docker/setup-qemu-action or docker/build-push-action instead?
💡 Result:
docker/setup-buildx-action@v3 does not support an input named "platforms"; platform configuration for multi-platform builds should instead be set via docker/build-push-action or docker/setup-qemu-action as appropriate.
-
docker/setup-buildx-action@v3: Its function is to install and configure Docker Buildx in your workflow; it does not define which platforms will be built[1][2][5][9]. Review of official documentation and usage examples consistently shows that the "platforms" input is not supported by this action. Its configurable inputs relate to builder settings (like version, buildkitd-flags, driver, driver-opts, etc.), not build target platforms[2][5][9].
-
docker/build-push-action: This action is responsible for building and optionally pushing Docker images, and it directly supports a platforms input. For example:
- name: Build and push uses: docker/build-push-action@v5 with: platforms: linux/amd64,linux/arm64
This input tells Buildx which platforms to build for[3][7].
-
docker/setup-qemu-action: This action is used to set up QEMU emulation, which is necessary for building images for certain platforms (like ARM on x86 runners). It supports an optional platforms input, but this refers to which emulators to install, not which platforms to build[3][7].
Summary of proper steps for multi-platform builds:
- Use docker/setup-buildx-action@v3 to set up the builder (no platforms input here).
- Use docker/setup-qemu-action@v3 to install emulators if building for architectures your runner does not natively support (optionally with its own "platforms" input to control which emulators to install)[3].
- Use docker/build-push-action (or the buildx CLI) with the "platforms" input to specify build target platforms[3][7].
This matches all examples and documentation seen across official sources and modern community guides[2][3][7][9].
Citations:
- 1: https://github.com/docker/setup-buildx-action
- 2: https://github.com/docker/setup-buildx-action/blob/master/README.md
- 3: https://cicube.io/workflow-hub/docker-setup-qemu-action/
- 4: https://github.com/docker/build-push-action
- 5: https://cicube.io/workflow-hub/docker-setup-buildx-action/
- 6: https://www.blacksmith.sh/blog/building-multi-platform-docker-images-for-arm64-in-github-actions
- 7: https://depot.dev/blog/multi-platform-docker-images-in-github-actions
- 8: https://forgejo.mixinet.net/docker/setup-buildx-action/src/commit/3472856dd98651ba887346cd57a440713d7af7f7
- 9: https://docs.docker.com/build/ci/github-actions/configure-builder/
- 10: https://docs.docker.com/build/building/multi-platform/
Remove unsupported platforms input from setup-buildx step
The docker/setup-buildx-action@v3 does not accept a platforms input. Specify build targets in the docker/build-push-action (or control emulators via docker/setup-qemu-action) instead.
• Location: .github/workflows/release.yml
– Line 36: under Set up Docker Buildx
• Action: Remove the platforms block from this step and move it to your build/push step.
Suggested diff:
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3
- with:
- platforms: linux/amd64,linux/arm64
# …later in your workflow…
- name: Build and push
uses: docker/build-push-action@v5
with:
platforms: linux/amd64,linux/arm64If you need QEMU emulation on x86 runners for ARM:
- name: Set up QEMU
uses: docker/setup-qemu-action@v3
with:
platforms: linux/amd64,linux/arm64🤖 Prompt for AI Agents
In .github/workflows/release.yml at line 36, the `platforms` input is
incorrectly used in the `docker/setup-buildx-action@v3` step, which does not
support it. Remove the `platforms` line from this step and instead specify the
target platforms in the `docker/build-push-action` step. If QEMU emulation is
needed for ARM on x86 runners, add a separate `docker/setup-qemu-action@v3` step
with the `platforms` input set accordingly.
There was a problem hiding this comment.
the buildx action does have this option: https://github.com/docker/setup-buildx-action#inputs
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the README from docker/setup-buildx-action to verify supported inputs
curl -s https://raw.githubusercontent.com/docker/setup-buildx-action/master/README.md | grep -A 50 "## Inputs"
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Fetch the action.yml which defines the actual inputs for the GitHub Action
curl -s https://raw.githubusercontent.com/docker/setup-buildx-action/master/action.yml | head -100
Length of output: 2982
@ghostdevv You're absolutely right, and I apologize for the incorrect information.
After checking the official action.yml file, I can confirm that docker/setup-buildx-action@v3 does support a platforms input. This input specifies "Fixed platforms for current node. If not empty, values take priority over the detected ones."
Your configuration with platforms: linux/amd64,linux/arm64 in the setup-buildx step is perfectly valid and correct for setting up a buildx instance that supports both architectures. I was mistaken in my initial review.
Thank you for the correction!
✏️ Learnings added
Learnt from: ghostdevv
PR: thushan/olla#46
File: .github/workflows/release.yml:36-36
Timestamp: 2025-08-09T18:03:17.928Z
Learning: The docker/setup-buildx-actionv3 GitHub Action supports a 'platforms' input parameter that specifies fixed platforms for the current buildx node. When provided, these values take priority over automatically detected platforms. This is valid configuration for multi-platform Docker builds.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| docker_manifests: | ||
| - name_template: ghcr.io/thushan/olla:latest | ||
| image_templates: | ||
| - "ghcr.io/thushan/olla:latest-amd64" | ||
| - "ghcr.io/thushan/olla:latest-arm64" | ||
| - name_template: ghcr.io/thushan/olla:{{ .Tag }} | ||
| image_templates: | ||
| - "ghcr.io/thushan/olla:{{ .Tag }}-amd64" | ||
| - "ghcr.io/thushan/olla:{{ .Tag }}-arm64" | ||
| - name_template: ghcr.io/thushan/olla:v{{ .Major }} | ||
| image_templates: | ||
| - "ghcr.io/thushan/olla:v{{ .Major }}-amd64" | ||
| - "ghcr.io/thushan/olla:v{{ .Major }}-arm64" | ||
| - name_template: ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }} | ||
| image_templates: | ||
| - "ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }}-amd64" | ||
| - "ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }}-arm64" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix YAML indentation under docker_manifests.image_templates (will trip yamllint and may confuse parsers)
YAMLlint flags wrong indentation at Lines 116, 120, 124, 128. The list items under image_templates are indented by 8 spaces instead of 6. Suggest correcting to 2-space increments.
Apply:
docker_manifests:
- name_template: ghcr.io/thushan/olla:latest
image_templates:
- - "ghcr.io/thushan/olla:latest-amd64"
- - "ghcr.io/thushan/olla:latest-arm64"
+ - "ghcr.io/thushan/olla:latest-amd64"
+ - "ghcr.io/thushan/olla:latest-arm64"
- name_template: ghcr.io/thushan/olla:{{ .Tag }}
image_templates:
- - "ghcr.io/thushan/olla:{{ .Tag }}-amd64"
- - "ghcr.io/thushan/olla:{{ .Tag }}-arm64"
+ - "ghcr.io/thushan/olla:{{ .Tag }}-amd64"
+ - "ghcr.io/thushan/olla:{{ .Tag }}-arm64"
- name_template: ghcr.io/thushan/olla:v{{ .Major }}
image_templates:
- - "ghcr.io/thushan/olla:v{{ .Major }}-amd64"
- - "ghcr.io/thushan/olla:v{{ .Major }}-arm64"
+ - "ghcr.io/thushan/olla:v{{ .Major }}-amd64"
+ - "ghcr.io/thushan/olla:v{{ .Major }}-arm64"
- name_template: ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }}
image_templates:
- - "ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }}-amd64"
- - "ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }}-arm64"
+ - "ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }}-amd64"
+ - "ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }}-arm64"You can verify locally with yamllint:
🏁 Script executed:
#!/bin/bash
pip install yamllint >/dev/null 2>&1 || true
yamllint .goreleaser.ymlLength of output: 894
Fix YAML indentation in docker_manifests.image_templates
YAMLlint reports wrong indentation for list items under docker_manifests.image_templates at lines 116, 120, 124 and 128 (expected 6 spaces, found 8). This will trip yamllint and may confuse other YAML parsers.
Affected locations:
- .goreleaser.yml:116
- .goreleaser.yml:120
- .goreleaser.yml:124
- .goreleaser.yml:128
Apply the following diff to correct indentation:
@@ .goreleaser.yml
docker_manifests:
- name_template: ghcr.io/thushan/olla:latest
image_templates:
- - "ghcr.io/thushan/olla:latest-amd64"
- - "ghcr.io/thushan/olla:latest-arm64"
+ - "ghcr.io/thushan/olla:latest-amd64"
+ - "ghcr.io/thushan/olla:latest-arm64"
@@ .goreleaser.yml
- name_template: ghcr.io/thushan/olla:{{ .Tag }}
image_templates:
- - "ghcr.io/thushan/olla:{{ .Tag }}-amd64"
- - "ghcr.io/thushan/olla:{{ .Tag }}-arm64"
+ - "ghcr.io/thushan/olla:{{ .Tag }}-amd64"
+ - "ghcr.io/thushan/olla:{{ .Tag }}-arm64"
@@ .goreleaser.yml
- name_template: ghcr.io/thushan/olla:v{{ .Major }}
image_templates:
- - "ghcr.io/thushan/olla:v{{ .Major }}-amd64"
- - "ghcr.io/thushan/olla:v{{ .Major }}-arm64"
+ - "ghcr.io/thushan/olla:v{{ .Major }}-amd64"
+ - "ghcr.io/thushan/olla:v{{ .Major }}-arm64"
@@ .goreleaser.yml
- name_template: ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }}
image_templates:
- - "ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }}-amd64"
- - "ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }}-arm64"
+ - "ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }}-amd64"
+ - "ghcr.io/thushan/olla:v{{ .Major }}.{{ .Minor }}-arm64"Verify locally with:
yamllint .goreleaser.yml🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 116-116: wrong indentation: expected 6 but found 8
(indentation)
[warning] 120-120: wrong indentation: expected 6 but found 8
(indentation)
[warning] 124-124: wrong indentation: expected 6 but found 8
(indentation)
[warning] 128-128: wrong indentation: expected 6 but found 8
(indentation)
🤖 Prompt for AI Agents
In .goreleaser.yml around lines 113 to 129, the list items under
docker_manifests.image_templates are indented with 8 spaces instead of the
expected 6 spaces, causing YAML linting errors. Fix this by reducing the
indentation of each image_templates list item from 8 spaces to 6 spaces to align
properly under image_templates. After adjusting the indentation, verify the YAML
file with yamllint to ensure no further indentation issues remain.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check if the YAML is valid despite the indentation
python3 -c "
import yaml
import sys
try:
with open('.goreleaser.yml', 'r') as f:
config = yaml.safe_load(f)
print('✅ YAML is valid and parseable')
# Check if docker_manifests section exists and has the expected structure
if 'docker_manifests' in config:
print(f'Found {len(config[\"docker_manifests\"])} docker manifest entries')
for i, manifest in enumerate(config['docker_manifests']):
if 'image_templates' in manifest:
print(f' Manifest {i+1}: {len(manifest[\"image_templates\"])} image templates')
except yaml.YAMLError as e:
print(f'❌ YAML parsing error: {e}')
sys.exit(1)
"
echo -e "\n--- Checking goreleaser validation ---"
# Check if goreleaser can validate the config
if command -v goreleaser &> /dev/null; then
goreleaser check --config .goreleaser.yml 2>&1 || true
else
echo "goreleaser not installed, checking examples in the repo..."
fi
echo -e "\n--- Checking other goreleaser configs for indentation patterns ---"
# Search for other docker_manifests examples in the codebase
rg -A 5 "docker_manifests:" --type yaml 2>/dev/null | head -20 || true
echo -e "\n--- Checking GoReleaser documentation examples ---"
# Let's check if there are any other YAML files that might show the pattern
fd -e yml -e yaml | xargs grep -l "image_templates" 2>/dev/null || true
Length of output: 1849
@ghostdevv, you're right to question this! The current 8-space indentation is functionally valid - the YAML parses correctly and GoReleaser will process it without issues.
This is primarily a style consistency issue flagged by yamllint. While fixing it to 6 spaces would align with YAML best practices and satisfy linters, it's not breaking anything as-is.
Feel free to leave it if you prefer - it's working fine! The choice is yours whether to prioritize linter compliance or keep the current working configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
internal/app/handlers/handler_status.go (3)
112-112: Bug: wrong error variable logged.Logs
errinstead ofemErr, masking the real failure from GetEndpointModelMap.- a.logger.Warn("Failed to get model map", "error", err) + a.logger.Warn("Failed to get model map", "error", emErr)
149-151: Guard against division by zero when no endpoints are configured.Avoids NaN/Inf and makes status classification explicit.
- healthyRatio := float64(len(healthy)) / float64(len(all)) + var healthyRatio float64 + if len(all) > 0 { + healthyRatio = float64(len(healthy)) / float64(len(all)) + }Optional: decide desired default status when
len(all) == 0(e.g. degraded) and set it explicitly.
93-94: Data race risk: global mutable issuesPool is shared across requests.
issuesPoolis mutated per request; concurrent requests can race.Make it local to the function:
- var issuesPool = make([]string, 0, 4) + // removed: use local slice in getEndpointIssues to avoid racesfunc (a *Application) getEndpointIssues(endpoint *domain.Endpoint, stats ports.EndpointStats, hasStats bool, successRate float64) string { - issuesPool = issuesPool[:0] + issues := make([]string, 0, 4) if endpoint.ConsecutiveFailures > 3 { - issuesPool = append(issuesPool, "consecutive failures") + issues = append(issues, "consecutive failures") } if hasStats { if successRate < 90.0 && stats.TotalRequests > 10 { - issuesPool = append(issuesPool, "low success rate") + issues = append(issues, "low success rate") } if stats.AverageLatency > 5000 { - issuesPool = append(issuesPool, "high latency") + issues = append(issues, "high latency") } } if endpoint.Status == domain.StatusOffline || endpoint.Status == domain.StatusUnhealthy { - issuesPool = append(issuesPool, "unavailable") + issues = append(issues, "unavailable") } - if len(issuesPool) == 0 { + if len(issues) == 0 { return emptyString } - return strings.Join(issuesPool, ", ") + return strings.Join(issues, ", ") }Also applies to: 258-283
🧹 Nitpick comments (2)
internal/app/handlers/handler_status.go (2)
8-8: Consider exposing GOOS alongside GOARCH for completeness.Given the PR adds arm64 builds, returning both runtime.GOARCH and runtime.GOOS makes the status payload more informative.
If you want this, add an OS field and populate it:
// in SystemSummary OS string `json:"os"` // in buildSystemSummary return OS: runtime.GOOS,
142-147: Sum total traffic across all endpoints, not just those with active connections.Current logic can undercount traffic for endpoints with no current connections.
- for url, conn := range connections { - totalConnections += conn - if stats, exists := endpointStats[url]; exists { - totalTraffic += stats.TotalBytes - } - } + for _, conn := range connections { + totalConnections += conn + } + for _, stats := range endpointStats { + totalTraffic += stats.TotalBytes + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/app/handlers/handler_process.go(3 hunks)internal/app/handlers/handler_status.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/app/handlers/handler_process.go
🧰 Additional context used
📓 Path-based instructions (1)
internal/{app,adapter}/**/*.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
Endpoints should be exposed at
/internal/healthand/internal/status.
Files:
internal/app/handlers/handler_status.go
🧠 Learnings (2)
📚 Learning: 2025-07-27T12:59:29.788Z
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.788Z
Learning: Applies to internal/adapter/proxy/*.go : Expose the following response headers: `X-Olla-Endpoint`, `X-Olla-Model`, `X-Olla-Backend-Type`, `X-Olla-Request-ID`, `X-Olla-Response-Time`.
Applied to files:
internal/app/handlers/handler_status.go
📚 Learning: 2025-07-27T12:59:29.787Z
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.787Z
Learning: Go version 1.24 or higher must be used.
Applied to files:
internal/app/handlers/handler_status.go
🧬 Code Graph Analysis (1)
internal/app/handlers/handler_status.go (1)
internal/version/version.go (1)
Version(18-18)
🔇 Additional comments (4)
internal/app/handlers/handler_status.go (4)
36-38: LGTM: Added fields are backwards compatible and clearly named.Adding
versionandarchto the status schema is sensible and non-breaking.
169-171: LGTM: Correctly populates version and arch.Values sourced from
version.Versionandruntime.GOARCHare appropriate for the new fields.
14-14: Version injection via ldflags is correctly configuredI’ve confirmed that your
.goreleaser.ymlincludes the necessary-Xflags to setversion.Version,Commit,DateandUserat build time, andinternal/version/version.godefines a defaultVersion = "v0.0.x"which will be overridden by those ldflags.• .goreleaser.yml (lines 49–54):
-s -w
-X "github.com/thushan/olla/internal/version.Version=v{{.Version}}"
-X "github.com/thushan/olla/internal/version.Commit={{.Commit}}"
-X "github.com/thushan/olla/internal/version.Date={{.CommitDate}}"
-X "github.com/thushan/olla/internal/version.User=goreleaser"• internal/version/version.go (line 18):
Version = "v0.0.x"No further action required here.
95-129: Route mapping for/internal/statusconfirmed
ThestatusHandleris correctly registered to/internal/statusin the routing setup—no further changes needed.• internal/app/handlers/server_routes.go:32 –
a.routeRegistry.RegisterWithMethod("/internal/status", a.statusHandler, "Endpoint status", "GET")
update makefile update version CLI with OS/Arch
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
.github/workflows/ci.yml (1)
91-128: Install GNU Make on Windows runners or skip make-based targetswindows-latest doesn’t include make by default; the “Build and test native platform” step will fail on Windows.
Add a Windows-only step before invoking make:
cross-platform: name: Cross-Platform Build Validation strategy: matrix: os: [ubuntu-latest, macos-latest, windows-latest] runs-on: ${{ matrix.os }} steps: - name: Checkout uses: actions/checkout@v4 - name: Configure Go uses: actions/setup-go@v5 with: go-version: ${{ env.GO_VERSION }} cache: true + - name: Install make on Windows + if: runner.os == 'Windows' + run: choco install make -y + shell: powershell + - name: Build and test native platform run: | if [ "${{ runner.os }}" = "Linux" ]; then make validate-linux elif [ "${{ runner.os }}" = "macOS" ]; then make validate-darwin elif [ "${{ runner.os }}" = "Windows" ]; then make validate-windows fi shell: bashAlternative: avoid make on Windows and run the equivalent go build commands inline. Happy to draft that if preferred.
🧹 Nitpick comments (2)
internal/version/version.go (1)
62-65: Clarify labels for OS/Arch and align consistently“For/On” can be ambiguous. Recommend explicit labels “OS” and “Arch” for readability.
Apply this diff for clearer output:
- b.WriteString(fmt.Sprintf(" For: %s\n", runtime.GOOS)) - b.WriteString(fmt.Sprintf(" On: %s\n", runtime.GOARCH)) + b.WriteString(fmt.Sprintf(" OS: %s\n", runtime.GOOS)) + b.WriteString(fmt.Sprintf(" Arch: %s\n", runtime.GOARCH))Optionally, for neat alignment irrespective of label length:
- b.WriteString(fmt.Sprintf(" OS: %s\n", runtime.GOOS)) - b.WriteString(fmt.Sprintf(" Arch: %s\n", runtime.GOARCH)) + b.WriteString(fmt.Sprintf(" %6s: %s\n", "OS", runtime.GOOS)) + b.WriteString(fmt.Sprintf(" %6s: %s\n", "Arch", runtime.GOARCH))makefile (1)
279-283: Help text minor typo/alignmentMissing space after “validate-windows” breaks alignment.
- @echo " validate-windows- Build and test Windows binaries (AMD64 + ARM64)" + @echo " validate-windows - Build and test Windows binaries (AMD64 + ARM64)"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.yml(1 hunks)internal/app/handlers/handler_status.go(3 hunks)internal/version/version.go(2 hunks)makefile(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/app/handlers/handler_status.go
🔇 Additional comments (2)
internal/version/version.go (1)
6-6: Importing runtime is appropriate hereGood addition to enable reporting of GOOS/GOARCH at runtime.
.github/workflows/ci.yml (1)
91-128: Cross-platform validation approach looks solidMatrix across Ubuntu/macOS/Windows plus post-build version check aligns well with the new multi-arch build goals.
|
okay, I've made some small changes to your awesome PR:
Thoughts? I'll suss out the yaml issues coderabbit mentions tomorrow, about to board a flight home. |
ghostdevv
changed the title
|
Sweet! I tweaked the pr title a bit, but I'm sure it could be refined further
I think this could be considered a bad practise considering it's volunteering potentially useful information to bad actors about the system and maybe versions (not sure what those fields contain) |
Yes very much so, we usually block all /internal endpoints from being visible outside of localhost but not sure its wise to allow this by default. Will rethink this. We have it in the CLI version tag anyway. There's a safety PR coming before next release that removes endpoints etc leftover, so I will review the rest there and tone this one down too. Good feedback thanks. |
thushan
left a comment
thushan
left a comment
There was a problem hiding this comment.
Looks great, thanks for your contribution!
2 participants
Closes #45
Summary by CodeRabbit
New Features
Enhancements
Documentation