Repository navigation
fix: Network.id now returns libpod lowercase 'id', add get_compatible… - #660
VincentSamuelPaul wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Restore the uppercase Id fallback and address the requested test coverage and lint issue.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Updates network ID handling for libpod’s lowercase id format while preserving Docker-compatible IDs for connect().
Changes:
- Uses lowercase
idforNetwork.id. - Adds
get_compatible_id()for compatible endpoints. - Updates connection handling and unit tests.
The uppercase Id fallback remains necessary for compatible network responses. Additional lowercase-ID coverage and trailing-whitespace cleanup are also required.
| File | Description |
|---|---|
podman/tests/unit/test_network.py |
Updates network ID tests and adds compatibility coverage. |
podman/domain/networks.py |
Implements libpod and compatible network ID handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| """str: Returns the identifier of the network.""" | ||
| with suppress(KeyError): | ||
| return self.attrs["Id"] | ||
| return self.attrs["id"] |
ef1d2c0 to
abcc397
Compare
|
The testing-farm:*:distro-fedora-all and testing-farm:fedora-44-x86_64:distro-sanity failures appear to be infrastructure errors unrelated to this change — they error out immediately with "1 plan errored out" and no test output, which is the same pattern seen in other recent PRs. No source logic was modified that would affect distro packaging or sanity checks. |
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
Thanks, I have some comments. Please also add in to commit Fixes: #NUMBER_OF_ISSUE. Thanks!
| """str: Returns the identifier of the network (Docker-compatible format).""" | ||
| with suppress(KeyError): | ||
| return self.attrs["Id"] | ||
|
|
There was a problem hiding this comment.
Can you please remove these white spaces?
There was a problem hiding this comment.
I've fixed it, removed trailing whitespace.
|
|
||
| return None | ||
|
|
||
| def get_compatible_id(self): |
There was a problem hiding this comment.
Should this be private function?
There was a problem hiding this comment.
Made it private (_get_compatible_id)
a55db8b to
9a2125e
Compare
Fixes: containers#584 Network.id was looking for self.attrs["Id"] (uppercase), but Podman's libpod API returns "id" (lowercase), causing it to always fall back to a synthetic hash-based ID. Changes: - Network.id now checks attrs["id"] first, then attrs["Id"] as fallback - Added private Network._get_compatible_id() for Docker-compatible endpoints - Network.connect() uses _get_compatible_id() for the NetworkID field - Updated test_id to cover both formats, added test_compatible_id Follows the approach suggested by @inknos in containers#587, analogous to how Network.name handles both "Name" and "name". Signed-off-by: Vincent Samuel Paul <vincentsamuelpaul@gmail.com>
9a2125e to
08216f1
Compare
|
Thanks for the review! |
Fixes #584
Network.idwas looking forself.attrs["Id"](uppercase), but Podman'slibpod API returns
"id"(lowercase), causing it to always fall back to asynthetic hash-based ID. Passing that fake ID to
networks.get()would then fail.Changes:
Network.idnow correctly readsattrs["id"](libpod format)Added
Network.get_compatible_id()for Docker-compatible endpoints that useattrs["Id"]Network.connect()usesget_compatible_id()since it goes through the Docker-compatible endpointUpdated
test_idto use libpod format, addedtest_compatible_idThis follows the approach suggested by @inknos in #587, analogous to how
Network.namealready handles both"Name"and"name".