client: NetworkInspect, NetworkList: wrap output structs, and remove NetworkInspectWithRaw#51245
client: NetworkInspect, NetworkList: wrap output structs, and remove NetworkInspectWithRaw#51245thaJeztah merged 2 commits intomoby:masterfrom
Conversation
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Docker client's network inspection and listing APIs by wrapping their return values in structured result types (NetworkInspectResult and NetworkListResult). The key changes include:
- Introducing
NetworkListResultstruct with anItemsfield containing network summaries - Introducing
NetworkInspectResultstruct withNetworkandRawfields - Removing the deprecated
NetworkInspectWithRawmethod in favor of the unifiedNetworkInspectthat returns both values - Updating all callsites throughout integration tests to access network data through the new wrapper structs
Reviewed Changes
Copilot reviewed 23 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| client/network_list.go | Introduces NetworkListResult struct and updates NetworkList to return it |
| client/network_inspect.go | Introduces NetworkInspectResult struct, removes NetworkInspectWithRaw, consolidates functionality into NetworkInspect |
| client/client_interfaces.go | Updates NetworkAPIClient interface to reflect new return types and method removal |
| client/network_list_test.go | Updates test to access network list via res.Items |
| client/network_inspect_test.go | Updates tests to access network data via res.Network |
| integration/* | Updates all integration test files to use new result struct fields (res.Items, res.Network) |
| internal/testutil/environment/*.go | Updates test utility functions to use new result struct fields |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| assert.Check(t, is.DeepEqual(wantSubnetStatus, res.Network.Status.IPAM.Subnets, cmpopts.EquateEmpty())) | ||
| } | ||
| cc.Close() | ||
| _ = cc.Close() |
There was a problem hiding this comment.
The error from cc.Close() is being explicitly discarded with _, but this is inconsistent with line 564 where Close() is called without discarding the error. Either handle the error consistently or add a comment explaining why it's safe to ignore in this specific case.
There was a problem hiding this comment.
Let me introduce you to reading diffs, CoPilot.
- red lines (and a
-at the first column) means "this code is remove", or updated ... - green lines (and a
+at the first column) means "this code is added, or updated"
| assert.Check(t, inspect.Status == nil) | ||
| cc.Close() | ||
| assert.Check(t, res.Network.Status == nil) | ||
| _ = cc.Close() |
There was a problem hiding this comment.
The error from cc.Close() is being explicitly discarded with _, but this is inconsistent with line 559 where Close() is called without assignment. Either handle the error consistently or add a comment explaining why it's safe to ignore in this specific case.
| type NetworkInspectResult struct { | ||
| Network network.Inspect |
There was a problem hiding this comment.
This one is slightly awkward but I don't have a good solution for that; the network.Inspect type is already a composite that embeds Network; you can access response.Network.ID directly, but of course also through response.Network.Network.ID
There was a problem hiding this comment.
Yuck, hmm thats something that can be improved in the next revision of the client. :)
- 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)