client/config: Wrap results and options#51078
Conversation
client/config_create.go
Outdated
|
|
||
| // ConfigCreateResult holds the result from the ConfigCreate method. | ||
| type ConfigCreateResult struct { | ||
| Response swarm.ConfigCreateResponse |
There was a problem hiding this comment.
Could use your thoughts on this; given that the API can be updated independent from the client module, we could have a situation where the API gained a new field, but the client may not handle it; in this case, likely not a huge issue as it directly unmarshals whatever it receives, but in other cases it could mean that the response is not handled, but the client returning a struct with fields not set and/or not handled correctly.
One option we have for that is to make the client define the counterpart to the response as well for the API version it was created for, e.g. similar to https://github.com/moby/moby/pull/50991/files
There was a problem hiding this comment.
Hmm tricky. I think this is only an issue if the user depends on a newer version of the api package than the vendored client package right?
Wondering how wide that use case would be, and is it important enough for us to mirror many API types in the client package.
There was a problem hiding this comment.
Yeah, definitely a tricky one; it's definitely possible it could happen if dependabot comes along and updates the API, although in many cases we may have a corresponding client release as well.
For some types we may want to do a conversion (at least the one I linked to exposes a reduced set of the fields on the client (TBD)).
One thing I did already account for is to make sure the client doesn't auto-magically "upgrade" by pinning the API version in the client module;
Lines 93 to 100 in 5c877f4
One option (to keep our options open) could be to define a local type that's a copy, not an alias, i.e.;
type ConfigCreateResponse swarm.ConfigCreateResponseBut I agree it's a thin line between handling this correctly and doing "too much" 🤔
There was a problem hiding this comment.
We probably could force the client package to depend on a specific api module version via replace rule 😅
Not sure if we should though...
There was a problem hiding this comment.
replace breaks go modules, or at least will cause go get to not work etc, so not a great option.
There was a problem hiding this comment.
Oh! And more importantly, "replace" is not seen by users of the module, so other than the above, they are ignored
fff361c to
4ec17cd
Compare
thaJeztah
left a comment
There was a problem hiding this comment.
overall LGTM, but left a comment
client/config_update.go
Outdated
|
|
||
| // ConfigUpdate attempts to update a config | ||
| func (cli *Client) ConfigUpdate(ctx context.Context, id string, version swarm.Version, config swarm.ConfigSpec) error { | ||
| func (cli *Client) ConfigUpdate(ctx context.Context, id string, options ConfigUpdateOptions) error { |
There was a problem hiding this comment.
for this one, I wonder if version should stay a positional arg, because it's really required.
Interested to hear what others think though /cc @corhere
4ec17cd to
7c66eaa
Compare
|
I rebased this one for the filters changes. I don't have opinions on the version argument though. :/ |
db0e8ea to
3dabd35
Compare
thaJeztah
left a comment
There was a problem hiding this comment.
Left some comments / thoughts ❤️ 🙈
f8dd6bd to
d875a89
Compare
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>
d875a89 to
abf5679
Compare
Uh oh!
There was an error while loading. Please reload this page.