Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,6 @@ jobs:
INTEGRATION_TEST_AZURE_CLIENT_SECRET: ${{ secrets.azure_client_secret }}
INTEGRATION_TEST_AZURE_CLIENT_ID: ${{ secrets.azure_client_id }}
KOSLI_SONAR_API_TOKEN: ${{ secrets.sonarqube_token }}
DOCKER_API_VERSION: "1.45"
KOSLI_API_TOKEN_PROD: ${{ secrets.kosli_querying_api_token }}
run: |
# some tests use git operations, therefore the git author on the CI VM needs to be set
Expand Down Expand Up @@ -273,4 +272,3 @@ jobs:
--trail ${{ inputs.TRAIL_NAME }}
--scan-results snyk-dependency.json
--org ${{ inputs.KOSLI_ORG }}

2 changes: 1 addition & 1 deletion cmd/kosli/snapshotDocker.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func (o *snapshotDockerOptions) run(args []string) error {

func CreateDockerArtifactsData() ([]*server.ServerData, error) {
result := []*server.ServerData{}
cli, err := client.NewClientWithOpts(client.FromEnv)
cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call site and DockerImageSha256 in internal/digest/digest.go still use client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) directly instead of going through the new newDockerClient() helper.

The PR description says the helper was introduced to "centralise the option so it cannot be accidentally omitted in future call sites," but these two call sites don't use it. Consider one of:

  1. Make newDockerClient() exported (NewDockerClient()) and call it from here and digest.go.
  2. Or accept the duplication and document the pattern in a comment.

Option 1 would fully deliver on the stated goal of centralisation.

Suggested change
cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
cli, err := docker.NewDockerClient()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call site and DockerImageSha256 in internal/digest/digest.go still construct the client inline rather than going through the newDockerClient() helper introduced in internal/docker/docker.go.

The PR description says the helper was introduced to "centralise the option so it cannot be accidentally omitted in future call sites," but two out of eight call sites don't use it. Consider exporting the helper as docker.NewDockerClient() and calling it from here and digest.go to fully deliver on that centralisation goal.

Suggested change
cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
cli, err := docker.NewDockerClient()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call site still constructs the Docker client inline rather than going through the newDockerClient() helper introduced in internal/docker/docker.go.

The PR description says the helper was introduced to "centralise the option so it cannot be accidentally omitted in future call sites," but this site (and DockerImageSha256 in digest.go) don't use it. Consider exporting the helper as docker.NewDockerClient() and calling it from both locations:

Suggested change
cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
cli, err := docker.NewDockerClient()

This would require adding the import and renaming/exporting the helper, but it fully delivers on the centralisation goal and prevents future drift.

if err != nil {
return result, err
}
Expand Down
4 changes: 2 additions & 2 deletions internal/digest/digest.go
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ func FileSha256(filepath string, logger *logger.Logger) (string, error) {
// It requires the docker daemon to be accessible and the docker image to be locally present.
// The docker image must have been pushed into a registry to have a digest.
func DockerImageSha256(imageID string) (string, error) {
cli, err := client.NewClientWithOpts(client.FromEnv)
cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
if err != nil {
return "", err
}
Expand Down Expand Up @@ -323,7 +323,7 @@ func requestManifestFromRegistry(registryEndPoint, imageName, imageTag, registry
// RemoteDockerImageSha256 returns a sha256 digest of a docker image by reading it from
// remote docker registry
func RemoteDockerImageSha256(imageName, imageTag, registryEndPoint, registryToken string, logger *logger.Logger) (string, error) {
// Some docker images have Manifest list, aka fat manifest which combines
// Some docker images have Manifest list, aka "fat manifest" which combines
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this line only has a trailing-whitespace change (removed a trailing space from the comment). Not harmful, but it's unrelated to the fix — consider dropping it to keep the diff focused on the version-negotiation change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this line is a whitespace-only change (trailing space removed from the comment). Not harmful, but unrelated to the fix — consider dropping it to keep the diff focused.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this line is a whitespace-only change (trailing space removed from the comment). Not harmful, but unrelated to the fix — consider dropping it to keep the diff focused.

// image manifests for one or more platforms. Other images don't have such manifest.
// The response Content-Type header specifies whether an image has it or not.
// More details here: https://docs.docker.com/registry/spec/manifest-v2-2/
Expand Down
20 changes: 14 additions & 6 deletions internal/docker/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,17 @@ import (
"github.com/docker/docker/client"
)

// newDockerClient creates a Docker client with API version negotiation enabled.
// This ensures the client automatically downgrades its API version to match
// the daemon, preventing "client version X is too new" errors when the SDK
// is newer than the Docker Engine.
func newDockerClient() (*client.Client, error) {
return client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
}

// PullDockerImage pulls a docker image or returns an error
func PullDockerImage(imageName string) error {
cli, err := client.NewClientWithOpts(client.FromEnv)
cli, err := newDockerClient()
if err != nil {
return err
}
Expand All @@ -41,7 +49,7 @@ func PullDockerImage(imageName string) error {

// PushDockerImage pushes a docker image to the local registry or returns an error
func PushDockerImage(imageName string) error {
cli, err := client.NewClientWithOpts(client.FromEnv)
cli, err := newDockerClient()
if err != nil {
return err
}
Expand Down Expand Up @@ -73,7 +81,7 @@ func PushDockerImage(imageName string) error {

// TagDockerImage tags a docker image or returns an error
func TagDockerImage(sourceName, targetName string) error {
cli, err := client.NewClientWithOpts(client.FromEnv)
cli, err := newDockerClient()
if err != nil {
return err
}
Expand All @@ -83,7 +91,7 @@ func TagDockerImage(sourceName, targetName string) error {

// RemoveDockerImage deletes a docker image or return an error
func RemoveDockerImage(imageName string) error {
cli, err := client.NewClientWithOpts(client.FromEnv)
cli, err := newDockerClient()
if err != nil {
return err
}
Expand All @@ -98,7 +106,7 @@ func RemoveDockerImage(imageName string) error {

// RunDockerContainer runs a docker container that sleeps for 6 minutes and returns its ID or returns an error
func RunDockerContainer(imageName string) (string, error) {
cli, err := client.NewClientWithOpts(client.FromEnv)
cli, err := newDockerClient()
if err != nil {
return "", err
}
Expand All @@ -117,7 +125,7 @@ func RunDockerContainer(imageName string) (string, error) {

// RemoveDockerContainer remove a docker container or returns an error
func RemoveDockerContainer(containerID string) error {
cli, err := client.NewClientWithOpts(client.FromEnv)
cli, err := newDockerClient()
if err != nil {
return err
}
Expand Down
Loading