network.id should look for attrs "Id" and "id"...

[nlacasse]

...before falling back to a synthetic id based on the hash of the network name.

Checking both is needed to support the Docker-compatible format ("Id") and Libpod-native format ("id").

This is analagous to how podman-py handles network.name -- we check attrs "Name" and "name" in that order.

Fixes #584

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nlacasse
Once this PR has been reviewed and has the lgtm label, please assign inknos for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[nlacasse]

Personally, I don't think we should ever generate a fake network id like this, but some tests depend on that behavior, so I left it in.

[inknos]

@nlacasse what about returning Id or id depending on the endpoint. the libpod api and docker's are essentially the same. they differ in the /libpod prefix. If this is a fix for the docker api only and breaks libpod behavior, I'd suggest to return it via the compatible endpoint

[nlacasse]

@nlacasse what about returning Id or id depending on the endpoint. the libpod api and docker's are essentially the same. they differ in the /libpod prefix. If this is a fix for the docker api only and breaks libpod behavior, I'd suggest to return it via the compatible endpoint

I'm not sure I understand the suggestion. Does the Network resource know whether is was created (or gotten) via a libpod endpoint? Or is the idea that when we initialize a new Network, we normalize the Id->id attr based on the endpoint before initailizing? That seems doable but might require a bit of plumbing.

Also, note that my change does not break docker (or any) behavior, it only fixes Network.id for libpod endpoints. The approach I took is the same way we handle docker/libpod compatability in Network.name. First we check for attrs["Name"] then attrs["name"]:
https://github.com/containers/podman-py/blob/main/podman/domain/networks.py#L54-L58

I think it's best to handle Network.id the same way we handle Network.name. Maybe we can move forward with this PR, since it fixes a bug, and later clean up the upper/lowercase compatability for both id and name together?

[inknos]

First of all, you are right. This is about libpod. I missed part of it.

To the fix!

I don't like to return lowercase id or uppercase Id, if that's a fix let's be as precise as possible and return the correct value. If podman wants id, we have to look for id. If docker wants Id we look for that.
Therefore, let's return id correctly in Network.id property and work from there.

Now, there are a few unit tests that should be inspected too.

---

podman.tests.unit.test_network.NetworkTestCase testMethod=test_id

If we want to be sure that libpod returns id, we have to change Id-> id. to be sure we are doing things right, we can also version the tests so it will pass conditionally to the new fix.

@@ -61,9 +61,9 @@ class NetworkTestCase(unittest.TestCase):
         self.addCleanup(self.client.close)
 
     def test_id(self):
-        expected = {"Id": "1cf06390-709d-4ffa-a054-c3083abe367c"}
+        expected = {"id": "1cf06390-709d-4ffa-a054-c3083abe367c"}
         actual = Network(attrs=expected)
-        self.assertEqual(actual.id, expected["Id"])
+        self.assertEqual(actual.id, expected["id"])
 
         actual = Network(attrs={"name": "database"})
         self.assertEqual(

Note that if you return "id" or "Id" you are not actually making any good use of this test, which would pass regardlessly. I would strongly like this test to fail strictly, to test for libpod. therefore we want this to return id or fail.

We are going to add test_compatible_id later for docker.

---

podman.tests.unit.test_network.NetworkTestCase testMethod=test_connect

this is more tricky, because this tests a docker payload which has Id as argument.

The Network.id call comes from inside Network.connect. something like this is usually handled in podman-py's style. by passing the compatible option to the request as a keyword argument, it returns a correct docker response.

@@ -99,6 +99,7 @@ class NetworkTestCase(unittest.TestCase):
             "podman_ctnr",
             aliases=["production"],
             ipv4_address="172.16.0.1",
+            compatible=True,
         )
         self.assertEqual(adapter.call_count, 1)
         self.assertDictEqual(

But with only the compatible change we get to an unwanted behavior. within Network.connect we make a Network.id cal, that cannot return "Id" or "id" based on a keyword arg. The solution to return either "Id" or "id" is not precise in this test.

How to fix it? turns out, some of the tests are wrong too! but with some more investigation and adding Network.get_compatible_id we can solve a lot of issues (maybe not all, but that's a more solid approach in my opinion)

Here's the whole patch which makes tox -e py -- podman/tests/unit/test_network.py pass.

0001-Patch-to-PR-587.patch

Let me know :)

[inknos]

I'd stick with the old behavior and simply return id. that's what we want.

[inknos]

does this test actually fail if id or Id?

[inknos]

hey, @nlacasse. Any update? do you need help or clarification from my side?