api/types/network: move network create/connect/disconnect options from api to client#50817
Conversation
thaJeztah
left a comment
There was a problem hiding this comment.
just some quick comments. overall LGTM, but I may need to give it a fresh look Tomorrow.
| Name string // Name is the requested name of the network. | ||
| Driver string // Driver is the driver-name used to create the network (e.g. `bridge`, `overlay`) | ||
| Scope string // Scope describes the level at which the network exists (e.g. `swarm` for cluster-wide or `local` for machine level). | ||
| EnableIPv4 *bool `json:",omitempty"` // EnableIPv4 represents whether to enable IPv4. | ||
| EnableIPv6 *bool `json:",omitempty"` // EnableIPv6 represents whether to enable IPv6. | ||
| IPAM *IPAM // IPAM is the network's IP Address Management. |
There was a problem hiding this comment.
As a follow-up, we should consider taking the opportunity to;
- Add
omitemptylabels to these fields - Add Names for the json fields; we've been too relaxed on that, and it's probably good to eplicitly set the names (also with their expected casing).
…dule Signed-off-by: Austin Vazquez <austin.vazquez@docker.com>
1027918 to
01479dc
Compare
| return clustertypes.NetworkCreateRequest{ | ||
| ID: nw.ID, | ||
| CreateRequest: network.CreateRequest{ | ||
| Name: name, // TODO(thaJeztah): this is the same as [nw.Spec.Annotations.Name]; consider using that instead | ||
| CreateOptions: options, | ||
| }, | ||
| ID: nw.ID, | ||
| CreateRequest: req, |
There was a problem hiding this comment.
Not for this PR, but now starting to wonder if we even need the intermediate CreateRequest here if it's gonna be set for the clustertypes.NetworkCreateRequest, but POSSIBLY we (or I?) did so to make sure we don't forget updating things in the cluster types when adding fields to the moby API etc.
| // WithDriver sets the driver of the network | ||
| func WithDriver(driver string) func(*network.CreateOptions) { | ||
| return func(n *network.CreateOptions) { | ||
| func WithDriver(driver string) func(*client.NetworkCreateOptions) { |
There was a problem hiding this comment.
This was always funny; our internal test client has a nicer API than the actual client. That said, I don't think the functional arguments here were all implemented correctly, so we should probably not adopt them as-is.
Signed-off-by: Austin Vazquez <austin.vazquez@docker.com>
01479dc to
1b4fcb8
Compare
|
Edited the second commit. Realized as CI was finishing up that I forgot to remove the create options type definition from api module. |
thaJeztah
left a comment
There was a problem hiding this comment.
oh! missed that as well
still LGTM, thanks!
- What I did
Partial for #50740
Related to #50786
This change moves
network.CreateOptions,network.ConnectOptions, andnetwork.DisconnectOptionsout of the api to the client module.- How I did it
- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)