Conversation
Replace hardcoded dockerClientVersion constant with configurable APIVersion field on DockerProvider. Adds --docker-api CLI flag and DOCKER_API env var (default "1.24") to support newer Docker engines that dropped old API versions (e.g., Docker 28+ requires 1.44+).
Coverage Report for CI Build 24386129258Coverage increased (+0.05%) to 86.756%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR makes the Docker Engine API version used by the Docker status provider configurable to avoid failures on newer Docker engines that no longer support the previously hardcoded API version.
Changes:
- Replace the hardcoded Docker API version with an
APIVersionfield onDockerProvider(defaulting to1.24when unset). - Add
--docker-api/DOCKER_APIand wire it throughmaininto the Docker provider. - Update Docker provider tests to cover both custom and default API versions, and ensure all
httptestservers are closed; document the new option in the README.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/status/external/docker_provider.go | Adds configurable API version handling (with a default when unset) to the Docker provider request path. |
| app/status/external/docker_provider_test.go | Updates tests to assert the configured API version is used and adds coverage for the default behavior; closes test servers. |
| app/main.go | Introduces --docker-api / DOCKER_API flag and wires the value into the Docker provider. |
| README.md | Documents the new CLI/env option and provides guidance for newer Docker engines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Concurrency int `long:"concurrency" env:"CONCURRENCY" default:"4" description:"number of concurrent requests to services"` | ||
| DockerAPIVersion string `long:"docker-api" env:"DOCKER_API" default:"1.24" description:"docker API version"` | ||
| Dbg bool `long:"dbg" env:"DEBUG" description:"show debug info"` |
There was a problem hiding this comment.
The CLI flag --docker-api accepts an arbitrary string and is only validated indirectly when the Docker request is built. To fail fast with a helpful message, consider validating opts.DockerAPIVersion after parsing (e.g., require major.minor format) and exiting with a clear error if invalid.
| --concurrency= number of concurrent requests to services (default: 4) [$CONCURRENCY] | ||
| --timeout= timeout for each request to services (default: 5s) [$TIMEOUT] | ||
| --timeout= timeout for each request to services (default: 5s) [$TIMEOUT] | ||
| --docker-api= docker API version (default: 1.24) [$DOCKER_API] |
There was a problem hiding this comment.
In the usage block, the --docker-api line has an extra space after = compared to the surrounding options. This looks like a formatting typo and makes the documented help output inconsistent; please align the spacing with the other lines.
| --docker-api= docker API version (default: 1.24) [$DOCKER_API] | |
| --docker-api= docker API version (default: 1.24) [$DOCKER_API] |
| apiVersion := d.APIVersion | ||
| if apiVersion == "" { | ||
| apiVersion = "1.24" | ||
| } | ||
| dkURL := fmt.Sprintf("http://localhost/v%s/containers/json", apiVersion) |
There was a problem hiding this comment.
APIVersion is user-configurable (CLI/env) but is interpolated directly into the request path. This allows invalid values (spaces, '/', '?', etc.) to produce malformed URLs or unexpected Docker endpoints and yields a generic request error. Consider normalizing (trim spaces, optionally strip leading 'v') and validating the version format (e.g., ^\d+\.\d+$) and returning a clear error when invalid.
The Docker provider hardcodes API version
1.24(Docker Engine 1.12 era). Newer Docker engines (28+, e.g. on Ubuntu 24.04) dropped support for API versions below1.44, causingsys-agentto fail with "client version 1.24 is too old" when querying/v1.24/containers/json.Changes:
dockerClientVersionconstant with configurableAPIVersionfield onDockerProvider--docker-apiCLI flag /DOCKER_APIenv var (default1.24)defer ts.Close()to all Docker test httptest servers