api/types/swarm: PortConfig: add Compare method#52047
Conversation
Add a compare function that can be used for slices.SortFunc to have a canonical definition of sorting. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
added
area/api
| if n := cmp.Compare(p.Protocol, other.Protocol); n != 0 { | ||
| return n |
There was a problem hiding this comment.
The only thing I wasn't sure of here was if we should perform "normalisation" here as well, because we have the explicit "not set means tcp", so two PortConfig's one with Protocol="" and one with Protocol="tcp" may have to be considered "equal"?
There was a problem hiding this comment.
From a quick look - I think Protocol is always populated in this one, so it's probably ok?
There was a problem hiding this comment.
@robmry yeah, I was considering having this utility to also be "usable" elsewhere; so that I can replace this in the CLI; not 100% sure if it's always propagated there
There was a problem hiding this comment.
I think swarm.PortConfig always comes from the daemon, which populates the field (it's never an API param, and I don't see where the default-TCP would be applied).
Maybe I'm missing something though. Assuming TCP in the ordering if it's not set couldn't really break anything.
There was a problem hiding this comment.
Pull request overview
This PR adds a Compare method to the PortConfig type in the Swarm API to enable canonical sorting of port configurations using slices.SortFunc. The method provides lexical ordering based on port and protocol properties.
Changes:
- Added
Compare(other PortConfig) intmethod toPortConfigstruct with ordering by PublishedPort, TargetPort, Protocol, and PublishMode - Added comprehensive test coverage in
network_test.godemonstrating the sorting behavior - Added
cmpimport to support the comparison implementation
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| api/types/swarm/network.go | Added Compare method to PortConfig with documentation of comparison priority order |
| vendor/github.com/moby/moby/api/types/swarm/network.go | Mirror of the Compare method implementation in vendor directory |
| api/types/swarm/network_test.go | New test file with TestPortConfigCompareSort verifying sorting behavior across different field combinations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (p PortConfig) Compare(other PortConfig) int { | ||
| if n := cmp.Compare(p.PublishedPort, other.PublishedPort); n != 0 { | ||
| return n | ||
| } | ||
| if n := cmp.Compare(p.TargetPort, other.TargetPort); n != 0 { | ||
| return n | ||
| } | ||
| if n := cmp.Compare(p.Protocol, other.Protocol); n != 0 { | ||
| return n | ||
| } | ||
| return cmp.Compare(p.PublishMode, other.PublishMode) | ||
| } |
There was a problem hiding this comment.
The Compare method does not include the Name field in the comparison. This means that two PortConfig instances with identical PublishedPort, TargetPort, Protocol, and PublishMode but different Name values would be considered equal by the comparison function, potentially resulting in non-deterministic sort order.
Consider adding Name as a fifth comparison criterion (after PublishMode) to ensure a fully deterministic ordering. This would make the comparison complete and prevent any ambiguity when sorting PortConfig slices.
There was a problem hiding this comment.
Name is not used currently (probably should be removed)
Add a compare function that can be used for slices.SortFunc to have a canonical definition of sorting.
- What I did
- 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)