-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Stop using deprecated SockRequest #33534
Stop using deprecated SockRequest #33534
Conversation
There are |
20e6a2a
to
9718178
Compare
@aaronlehmann should be fixed now |
looks like windowsRS1 run out of space. not sure what to do with: |
Looks like a couple of tests are failing |
@thaJeztah failing tests are counting leaked goroutines. I'm not sure what caused that, the only change around those tests is that getGoroutineNumber() now uses client.Info() instead of SockRequest call to "/info" |
@sbko Seems like there is something wrong with the client usage. |
@cpuguy83 looks like it. I just reverted change to getGoroutineNumber() and tests passed. I tried requests.Get("/info") instead of SockRequests("GET", "/info,..) and it passes tests as well. I will take a look at client library. |
@cpuguy83 Looks like it's client itself. |
5cdde3b
to
341fe69
Compare
@thaJeztah tests are passing now |
17f8658
to
99426d6
Compare
any changes to api?
|
I think I know what's happening; version negotiation is currently in the cli, not in the API-client, hence it doesn't know what version of the daemon it's talking to. There's a pull request to update that; #32779 |
But, hm.. if no version was found, it should probably assume "latest" and have the API server return the appropriate error if needed. This is the code that generates the error; Lines 228 to 235 in 69c35da
|
@thaJeztah do we need to open an issue for that? |
@thaJeztah sure. Thanks! |
99426d6
to
648c01b
Compare
@vdemeester PTAL. do I need to keep rebasing or wait for reviews first? |
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.
SGTM 🐸
@sbko You can rebase 😉
c58caaa
to
d7c1f6a
Compare
@vdemeester done! :) |
@thaJeztah what should be my next step? just wait? |
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.
LGTM (sorry for the delay)
ping @dnephin can you have another look as well?
I see it needs a minor rebase
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 PR is quite large. I did another quick scan, and it generally seems fine, but I didn't really look at every change.
In the future doing multiple smaller PRs will make it much easier to review, and should reduce the time it takes to get it merged.
Couple minor comments, but nothing blocking a merge.
integration-cli/daemon/daemon.go
Outdated
} | ||
|
||
cli, err := client.NewClient(d.Sock(), api.DefaultVersion, httpClient, nil) | ||
if err != nil { |
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.
minor: this if
is unnecessary, just return client.NewClient(...)
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.
@dnephin done
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.
I still see the if here
if status != http.StatusOK { | ||
return info.Swarm, fmt.Errorf("get swarm info: invalid statuscode %v", status) | ||
} | ||
cli, err := d.NewClient() |
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.
It might be easier to just store a client on the daemon
struct here, instead of recreating it each time.
I guess that's an improvement we can make in the future.
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.
ok
d7c1f6a
to
5824d79
Compare
@thaJeztah I rebased the code, added requested change and tests failed with looks like unrelated errors. I see that other PRs experience similar errors. is that a known issue? thanks! |
I think there was an issue with Docker Hub search causing these failures; restarting CI |
@thaJeztah looks like still failing:
Error should be "No container name or ID supplied" but for some reason it's "Error response from daemon: page not found". Checking why is that... |
@sbko yes, I chatted with the Hub team yesterday, and the cause was that the search index was out of date, causing the They triggered a re-index, and it should now (really) be fixed; triggering once more 😅 |
@vdemeester I was busy last week. I will probably rebase tonight or tomorrow. |
9f810ec
to
2332f77
Compare
@vdemeester @dnephin rebased |
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 might be easier to get in (faster reveiws and fewer rebases) if it were split into smaller chunks. +/- 300 lines is significantly easier to review than +/-1500.
) | ||
|
||
func (s *DockerSuite) TestAPICreateWithNotExistImage(c *check.C) { |
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 looks like a bad merge. These tests were removed in #34000
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.
Done
integration-cli/environment/clean.go
Outdated
func getAllPlugins() (types.PluginsListResponse, error) { | ||
var plugins types.PluginsListResponse | ||
_, b, err := request.SockRequest("GET", "/plugins", nil, request.DaemonHost()) | ||
func getAllPlugins(c *client.Client) (types.PluginsListResponse, error) { |
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.
I think this should be using client.APIClient
(the interface), same is true in other places.
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.
Done
cli, err := client.NewEnvClient() | ||
if err != nil { | ||
t.Fatalf("%v", err) | ||
} |
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.
require.NoError(t, err)
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.
looks like "require.NoError" doesn't work with "testingT"
integration-cli/daemon/daemon.go
Outdated
} | ||
|
||
cli, err := client.NewClient(d.Sock(), api.DefaultVersion, httpClient, nil) | ||
if err != nil { |
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.
I still see the if here
Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>
2332f77
to
0fd5a65
Compare
Signed-off-by: Stanislav Bondarenko stanislav.bondarenko@gmail.com
per #31410
Use
client/
package instead ofrequest
deprecatedSockRequest
. If not viable to useclient/
replace with newDo
,Post
,Get
, ...- Description for the changelog
integration-cli: Use
client/
package instead of deprecatedSockRequest