Add configurable Docker API version flag#241
Conversation
Adds --docker.api-version flag (env: DOCKER_API_VERSION) to allow users to specify the Docker API version. Defaults to 1.24 for backward compatibility. Users on Docker 29+ should set this to 1.44. Related to #240
There was a problem hiding this comment.
Pull request overview
This PR adds a configurable Docker API version flag to allow users to specify which Docker API version to use for container discovery, addressing compatibility needs for different Docker versions.
- Added
--docker.api-versionflag with environment variableDOCKER_API_VERSION - Set default API version to
1.24for backward compatibility with Docker 1.12+ - Updated Docker client initialization to accept and use the configurable API version parameter
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/main.go | Added APIVersion field to Docker options struct with default value "1.24" and passed it to NewDockerClient |
| app/discovery/provider/docker.go | Updated dockerClient struct and NewDockerClient function to accept and store API version; modified ListContainers to use configurable version with "v" prefix |
| app/discovery/provider/docker_test.go | Updated test cases to pass API version parameter "1.44" to NewDockerClient calls |
| README.md | Added documentation for the new --docker.api-version flag |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const APIVersion = "v1.24" | ||
|
|
||
| resp, err := d.client.Get(fmt.Sprintf("http://localhost/%s/containers/json", APIVersion)) | ||
| apiVersion := "v" + d.apiVersion |
There was a problem hiding this comment.
The code unconditionally prepends "v" to the API version, which could lead to malformed version strings if a user accidentally provides the version with the "v" prefix already included (e.g., "v1.24" would become "vv1.24"). Consider either: (1) validating/sanitizing the input to strip any leading "v" before prepending it, or (2) documenting clearly that the version should be provided without the "v" prefix and adding validation to reject versions that include it.
| apiVersion := "v" + d.apiVersion | |
| apiVersion := d.apiVersion | |
| if !strings.HasPrefix(apiVersion, "v") { | |
| apiVersion = "v" + apiVersion | |
| } |
| addr := fmt.Sprintf("tcp://%s", strings.TrimPrefix(srv.URL, "http://")) | ||
|
|
||
| client := NewDockerClient(addr, "bridge") | ||
| client := NewDockerClient(addr, "bridge", "1.44") |
There was a problem hiding this comment.
The test has been changed to use API version "1.44", but the default version in the production code is "1.24". Tests should ideally validate the default behavior unless there's a specific reason to test a different version. Consider either testing with "1.24" to match the default, or adding an additional test case specifically for version "1.44" while keeping a test for the default version.
Summary
Adds
--docker.api-versionflag (env:DOCKER_API_VERSION) to allow users to specify the Docker API version used for container discovery.1.24(backward compatible with Docker 1.12+)DOCKER_API_VERSION=1.44Changes
APIVersionfield to Docker options in main.goNewDockerClientto accept API version parameterdockerClient.ListContainersto use configurable versionRelated to #240