client: Migrate tests to use functional opts and extract clientConfig
#50847
Conversation
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
All of its usage was replaced by WithMockClient Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Change functional options for the client so that they operate on an intermediate struct instead of the public `Client` instance directly. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
| err = client.CheckpointCreate(context.Background(), "nothing", checkpoint.CreateOptions{ | ||
| CheckpointID: "noting", |
There was a problem hiding this comment.
Heh; interesting; looks like CheckpointID had a typo? (noting instead of nothing?) or was that intentional to produce the error? 🤔
There was a problem hiding this comment.
It doesn't really matter - the nothing here is the container ID, and notin is the checkpoint ID.
| // performed on the first request, after which negotiated is set to "true" | ||
| // so that subsequent requests do not re-negotiate. | ||
| negotiateVersion bool | ||
| clientConfig |
There was a problem hiding this comment.
The only thing I was considering was if we needed to keep the “config” struct separate from the “active” config, but I had to look at the actual code to see if that made sense.
i.e. considering if WithFoo() would update the config struct, but the client fields to be updated from that config; I was mostly wondering if there would be cases where applying the configs could not be done fully atomically, and if an intermediate struct (to be applied) would be good for that.
But I haven’t given that much thought yet; that was only thinking out loud.
There was a problem hiding this comment.
Currently all of these are used later by the Client (except the traceOpts), so it would basically require duplicating almost whole struct inside the client.
IMO, too much churn for little benefit at this point. Should we need that in future, this is easier with this PR because the config struct is private and the whole logic is the implementation detail of the Client so we can do everything we want.
There was a problem hiding this comment.
Should we need that in future, this is easier with this PR because the config struct is private and the whole logic is the implementation detail of the Client so we can do everything we want.
Yeah, that's what I concluded as well; if we need to / decide to make such a change, we can do so without breaking compatibility, so big win there.
WithMockClientfor the unit tests the create a client with mocked http transport (instead of creating theClientstruct directly and messing with internal fields)client.Optto mutate an internal config field instead of the wholeClientinstance- How to verify it
CI