Skip to content
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.

Sign up for GitHub

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

Jump to bottom

testutil: copy slim version of the docker client into testutil #5731

Merged
merged 1 commit into from
Feb 24, 2025
Merged

testutil: copy slim version of the docker client into testutil #5731

merged 1 commit into from
Feb 24, 2025

Conversation

jsternberg
Copy link
Collaborator

Copies a slim version of the docker client with only the necessary methods so we can break our dependency on the client in moby. This client is only used in an integration test so it's not really needed and we don't really actively need updates or to be on the most recent API version since we just do an unversioned ping and then call the hijack method.

This was created by copying the package into testutil and then deleting unused sections of code.

Fixes #3355.

@github-actions github-actions bot added area/dependencies Pull requests that update a dependency file area/util labels Feb 12, 2025
@jsternberg jsternberg force-pushed the remove-docker-api-dependency branch 2 times, most recently from 723c3dc to 85a8590 Compare February 12, 2025 19:01
@jsternberg jsternberg marked this pull request as ready for review February 12, 2025 20:09
@jsternberg jsternberg force-pushed the remove-docker-api-dependency branch from 85a8590 to 2d26d6c Compare February 12, 2025 21:49
@tonistiigi
Copy link
Member

@jsternberg Can you open a draft PR from this branch in moby so we can see the tests with dockerd worker passing there.

@jsternberg jsternberg mentioned this pull request Feb 13, 2025
Draft
@jsternberg jsternberg force-pushed the remove-docker-api-dependency branch from 2d26d6c to 35d84a6 Compare February 13, 2025 16:46
@jsternberg
Copy link
Collaborator Author

jsternberg commented Feb 13, 2025 •
edited
Loading

This is passing in the draft PR. I had to rebase this PR to be based on v0.19 though to make it work because of unrelated API changes. I'll rebase it back and resolve the conflicts.

moby/moby#49459

@jsternberg jsternberg force-pushed the remove-docker-api-dependency branch from 35d84a6 to 2cea406 Compare February 13, 2025 19:01
@jsternberg jsternberg force-pushed the remove-docker-api-dependency branch from 2cea406 to f81eeb4 Compare February 14, 2025 15:34
tonistiigi
tonistiigi reviewed Feb 20, 2025
View reviewed changes
@jsternberg jsternberg force-pushed the remove-docker-api-dependency branch from f81eeb4 to 0c8625f Compare February 20, 2025 15:33
@jsternberg
Copy link
Collaborator Author

@tonistiigi applied the feedback.

@thompson-shaun thompson-shaun added this to the v0.21.0 milestone Feb 20, 2025
tonistiigi
tonistiigi reviewed Feb 20, 2025
View reviewed changes
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Looks good. Rerun the Moby PR after updating as well.

util/testutil/dockerd/client/options.go Outdated
c.proto = hostURL.Scheme
c.addr = hostURL.Host
c.basePath = hostURL.Path
if tr, ok := c.client.Transport.(*http.Transport); ok {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to do anything based on host, so can be in the default constructor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried changing this but it does seem to matter. It does things based on the proto and host and has been the cause of the new failures so I'm going to revert this back to what's in the original client.

@jsternberg jsternberg force-pushed the remove-docker-api-dependency branch 2 times, most recently from 0b451d4 to 418b39a Compare February 20, 2025 19:06
tonistiigi
tonistiigi reviewed Feb 20, 2025
View reviewed changes
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Code looks ok, but seems latest PR in moby has some issues. I see errors from ping there.

util/testutil/dockerd/client/client.go Outdated
return nil, errors.New("No host URL specified in client")
}

if tr, ok := c.client.Transport.(*http.Transport); ok {
Copy link
Member

Choose a reason for hiding this comment

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

Not a big issue, but this type-cast still seems unnecessary. *http.Transport comes directly from defaultHTTPClient so you could just call defaultHTTPTransport instead to get it with the correct type.

@jsternberg jsternberg force-pushed the remove-docker-api-dependency branch from 418b39a to 414325d Compare February 21, 2025 16:35
@tonistiigi
Copy link
Member

@jsternberg Check the Moby PR

@jsternberg jsternberg force-pushed the remove-docker-api-dependency branch from 0b6bebd to 15decde Compare February 24, 2025 16:27
@jsternberg jsternberg force-pushed the remove-docker-api-dependency branch from 15decde to 2dee7fe Compare February 24, 2025 16:30
@jsternberg
testutil: copy slim version of the docker client into testutil
0a7f949 
Copies a slim version of the docker client with only the necessary
methods so we can break our dependency on the client in moby. This
client is only used in an integration test so it's not really needed and
we don't really actively need updates or to be on the most recent API
version since we just do an unversioned ping and then call the hijack
method.

This was created by copying the package into `testutil` and then
deleting unused sections of code.

Signed-off-by: Jonathan A. Sternberg <[email protected]>
@jsternberg jsternberg force-pushed the remove-docker-api-dependency branch from 2dee7fe to 0a7f949 Compare February 24, 2025 16:53
@tonistiigi tonistiigi merged commit a20380c into moby:master Feb 24, 2025
105 checks passed
@tonistiigi
Copy link
Member

In follow-up we should remove the go-connections/sockets import. Looks like only use the function that forces the proto+addr to the dialer (so it doesn't come from request). We can just copy that short snippet. The default timeout in go-connections can be in defaults in here (if it does anything at all).

@jsternberg jsternberg mentioned this pull request Feb 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi left review comments

@thaJeztah thaJeztah thaJeztah left review comments

@crazy-max crazy-max crazy-max approved these changes

Assignees

@tonistiigi tonistiigi

Labels
area/dependencies Pull requests that update a dependency file area/util needs/follow-up
Projects
None yet
Milestone
v0.21.0
Development

Successfully merging this pull request may close these issues.

remove dependency on docker/docker/client
5 participants
@jsternberg @tonistiigi @thaJeztah @crazy-max @thompson-shaun