-
Notifications
You must be signed in to change notification settings - Fork 6
fix: add Docker API version negotiation to prevent client/daemon mismatch #761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
216ac87
c42e2ab
09981bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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()) | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This call site and 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
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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
Suggested change
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 | ||||||||||
| } | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| } | ||
|
|
@@ -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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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/ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call site and
DockerImageSha256ininternal/digest/digest.gostill useclient.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())directly instead of going through the newnewDockerClient()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:
newDockerClient()exported (NewDockerClient()) and call it from here anddigest.go.Option 1 would fully deliver on the stated goal of centralisation.