chore: Upgrade dependencies - primarily addressing breaking changes in docker packages #9927
Conversation
…n docker packages
Summary of ChangesHello @mattsanta, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on upgrading various dependencies in the project, primarily to address breaking changes in Docker packages and to keep the project up-to-date with the latest library versions. The updates also include code modernization by migrating to the new Moby client, enhancing compatibility and stability. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
…n docker packages
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades a significant number of dependencies, most notably migrating from the docker/docker client to the moby/moby client. While most of the changes correctly adapt to the new APIs, I've found several issues related to incorrect API usage with the new Moby client, which will lead to compilation errors or incorrect behavior. These issues are primarily in pkg/skaffold/docker/image.go and need to be addressed. I've also pointed out some incorrect struct field access in the integration tests due to the API changes.
| // Delete stops, removes, and prunes a running container | ||
| func (l *localDaemon) Delete(ctx context.Context, out io.Writer, id string) error { | ||
| if err := l.apiClient.ContainerStop(ctx, id, container.StopOptions{}); err != nil { | ||
| if _, err := l.apiClient.ContainerStop(ctx, id, client.ContainerStopOptions{}); err != nil { |
There was a problem hiding this comment.
The ContainerStop function from the Moby client returns only an error. Using a blank identifier _ for a non-existent return value will cause a compilation error.
| if _, err := l.apiClient.ContainerStop(ctx, id, client.ContainerStopOptions{}); err != nil { | |
| if err := l.apiClient.ContainerStop(ctx, id, client.ContainerStopOptions{}); err != nil { |
| log.Entry(ctx).Debugf("unable to stop running container: %s", err.Error()) | ||
| } | ||
| if err := l.apiClient.ContainerRemove(ctx, id, container.RemoveOptions{}); err != nil { | ||
| if _, err := l.apiClient.ContainerRemove(ctx, id, client.ContainerRemoveOptions{}); err != nil { |
There was a problem hiding this comment.
The ContainerRemove function from the Moby client returns only an error. Using a blank identifier _ for a non-existent return value will cause a compilation error.
| if _, err := l.apiClient.ContainerRemove(ctx, id, client.ContainerRemoveOptions{}); err != nil { | |
| if err := l.apiClient.ContainerRemove(ctx, id, client.ContainerRemoveOptions{}); err != nil { |
|
|
||
| func (l *localDaemon) NetworkRemove(ctx context.Context, name string) error { | ||
| return l.apiClient.NetworkRemove(ctx, name) | ||
| _, err := l.apiClient.NetworkRemove(ctx, name, client.NetworkRemoveOptions{}) |
There was a problem hiding this comment.
The NetworkRemove function in the Moby client returns only an error and does not accept client.NetworkRemoveOptions. The blank identifier _ and the options argument are incorrect and will cause a compilation error.
| _, err := l.apiClient.NetworkRemove(ctx, name, client.NetworkRemoveOptions{}) | |
| return l.apiClient.NetworkRemove(ctx, name) |
| if _, err := l.apiClient.ImageTag(ctx, client.ImageTagOptions{Source: image, Target: ref}); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The ImageTag function in the Moby client expects source and target strings as arguments, not an options struct. This will cause a compilation error.
return l.apiClient.ImageTag(ctx, image, ref)| so.Timeout = util.Ptr[int](int(stopTimeout.Seconds())) | ||
| } | ||
| if err := l.apiClient.ContainerStop(ctx, id, so); err != nil { | ||
| if _, err := l.apiClient.ContainerStop(ctx, id, so); err != nil { |
There was a problem hiding this comment.
The ContainerStop function from the Moby client returns only an error. Using a blank identifier _ for a non-existent return value will cause a compilation error.
if err := l.apiClient.ContainerStop(ctx, id, so); err != nil {|
|
||
| func (l *localDaemon) Remove(ctx context.Context, id string) error { | ||
| if err := l.apiClient.ContainerRemove(ctx, id, container.RemoveOptions{}); err != nil { | ||
| if _, err := l.apiClient.ContainerRemove(ctx, id, client.ContainerRemoveOptions{}); err != nil { |
There was a problem hiding this comment.
The ContainerRemove function from the Moby client returns only an error. Using a blank identifier _ for a non-existent return value will cause a compilation error.
if err := l.apiClient.ContainerRemove(ctx, id, client.ContainerRemoveOptions{}); err != nil {| containers, err := dockerclient.ContainerList(context.Background(), client.ContainerListOptions{All: true}) | ||
| if err != nil { | ||
| t.Fail() | ||
| } | ||
| time.Sleep(sleepTime) | ||
| if len(containers) == 0 { | ||
| if len(containers.Items) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| checkEntrypointRewrite(containers, &verifyEntrypointRewrite) | ||
| checkSupportContainer(containers, &verifySupportContainer) | ||
| checkEntrypointRewrite(containers.Items, &verifyEntrypointRewrite) | ||
| checkSupportContainer(containers.Items, &verifySupportContainer) |
There was a problem hiding this comment.
The result of ContainerList from the new Moby client is a struct that contains the list of containers in the Items field. The code needs to be updated to access this field to get the list of containers.
| containers, err := dockerclient.ContainerList(context.Background(), client.ContainerListOptions{All: true}) | |
| if err != nil { | |
| t.Fail() | |
| } | |
| time.Sleep(sleepTime) | |
| if len(containers) == 0 { | |
| if len(containers.Items) == 0 { | |
| continue | |
| } | |
| checkEntrypointRewrite(containers, &verifyEntrypointRewrite) | |
| checkSupportContainer(containers, &verifySupportContainer) | |
| checkEntrypointRewrite(containers.Items, &verifyEntrypointRewrite) | |
| checkSupportContainer(containers.Items, &verifySupportContainer) | |
| containers, err := dockerclient.ContainerList(context.Background(), client.ContainerListOptions{All: true}) | |
| if err != nil { | |
| t.Fail() | |
| } | |
| time.Sleep(sleepTime) | |
| if len(containers.Items) == 0 { | |
| continue | |
| } | |
| checkEntrypointRewrite(containers.Items, &verifyEntrypointRewrite) | |
| checkSupportContainer(containers.Items, &verifySupportContainer) |
| if cInfo.Container.State.Running { | ||
| containersRunning++ | ||
| } | ||
|
|
||
| if cInfo.State.Dead || cInfo.State.Restarting { | ||
| if cInfo.Container.State.Dead || cInfo.Container.State.Restarting { | ||
| return false, fmt.Errorf("container %v is in dead or restarting state", cn) |
There was a problem hiding this comment.
The ContainerInspect function returns a types.ContainerJSON object which has a State field directly. Accessing State via a Container field is incorrect and will fail. It should be cInfo.State.
| if cInfo.Container.State.Running { | |
| containersRunning++ | |
| } | |
| if cInfo.State.Dead || cInfo.State.Restarting { | |
| if cInfo.Container.State.Dead || cInfo.Container.State.Restarting { | |
| return false, fmt.Errorf("container %v is in dead or restarting state", cn) | |
| if cInfo.State != nil && cInfo.State.Running { | |
| containersRunning++ | |
| } | |
| if cInfo.State != nil && (cInfo.State.Dead || cInfo.State.Restarting) { | |
| return false, fmt.Errorf("container %v is in dead or restarting state", cn) | |
| } |
| return nil, errors.Wrap(err, "inspecting init container") | ||
| } | ||
| if len(r.Mounts) != 1 { | ||
| if len(r.Container.Mounts) != 1 { |
There was a problem hiding this comment.
The result of ContainerInspect is a types.ContainerJSON object which has a Mounts field directly. Accessing Mounts via a Container field is incorrect. It should be r.Mounts.
| if len(r.Container.Mounts) != 1 { | |
| if len(r.Mounts) != 1 { |
| return err | ||
| } | ||
| if r.Warning != "" { | ||
| if len(r.Warning) == 0 { |
There was a problem hiding this comment.
This condition appears to be inverted. It logs a warning if r.Warning is empty. It should likely log a warning when r.Warning is not empty.
| if len(r.Warning) == 0 { | |
| if len(r.Warning) > 0 { |
…rTools#9917) Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.43.0 to 0.45.0. - [Commits](golang/crypto@v0.43.0...v0.45.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-version: 0.45.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…n docker packages
1 participant
chore: Upgrade dependencies - primarily addressing breaking changes in docker packages
Still WIP. Needs some dependencies to also address the breaking changes.
google/go-containerregistry#2160
buildpacks/pack#2470
buildpacks/imgutil#298