Consolidate api port types#51094
Consolidate api port types#51094thaJeztah merged 3 commits intomoby:mastermoby/moby:masterfrom austinvazquez:consolidate-api-port-typesaustinvazquez/moby:consolidate-api-port-typesCopy head branch name to clipboard
Conversation
|
other PRs are merged, so this one can be rebased 😅 |
|
Oh! In case it's needed to help prevent circular imports; we also have a (not saying we should use that, but in case |
Signed-off-by: Austin Vazquez <austin.vazquez@docker.com>
Signed-off-by: Austin Vazquez <austin.vazquez@docker.com>
Signed-off-by: Austin Vazquez <austin.vazquez@docker.com>
783832c to
50269e6
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates port-related types by moving container port and port range types from the container package to the network package, deprecating the use of swarm.PortConfigProtocol in favor of the consolidated types.
- Moved port types (
Port,PortRange,PortMap,PortBinding,PortSet) fromapi/types/containertoapi/types/network - Replaced
swarm.PortConfigProtocolwithnetwork.IPProtocolthroughout the codebase - Updated all imports and references to use the consolidated network package types
Reviewed Changes
Copilot reviewed 44 out of 49 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| api/types/network/port.go | Moved from container package, renamed NetworkProtocol to IPProtocol |
| api/types/network/port_test.go | Moved from container package with updated package declaration |
| api/types/container/config.go | Updated to use network.PortSet for ExposedPorts |
| api/types/container/hostconfig.go | Updated to use network.PortMap for PortBindings |
| api/types/container/network_settings.go | Updated to use network.PortMap for Ports |
| api/types/swarm/network.go | Replaced PortConfigProtocol with network.IPProtocol |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
@akerouanton @robmry PTAL
| const ( | ||
| TCP NetworkProtocol = "tcp" | ||
| UDP NetworkProtocol = "udp" | ||
| SCTP NetworkProtocol = "sctp" | ||
| TCP IPProtocol = "tcp" | ||
| UDP IPProtocol = "udp" | ||
| SCTP IPProtocol = "sctp" |
There was a problem hiding this comment.
As a follow-up, perhaps we should add a comment to this block to;
- Outline these are the protocols currently supported by the daemon
- (?) But that the list may be non-exhaustive (at least, I recall some discussion about the daemon being the source of truth for this, not the API)
|
All green; let's bring this one in 👍 |
- What I did
Follow-up to #50710
- How I did it
Moved the container port/port range types under network package and deprecated and removed the usage of swarm.PortConfigProtocol.
- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)