fix: make service connection identity optional by pratham8499 · Pull Request #656 · containers/podman-py

pratham8499 · 2026-09-21T22:57:06Z

Before the fix:
a) PodmanConfig().services['local'].identity
-> failed: TypeError expected str, bytes or os.PathLike object, not NoneType
b) PodmanClient(connection='local')
-> failed: TypeError expected str, bytes or os.PathLike object, not NoneType
c) PodmanClient(connection='local', identity='')
-> failed: TypeError expected str, bytes or os.PathLike object, not NoneType
d) PodmanClient(connection='production')
-> success

After the fix:
a) PodmanConfig().services['local'].identity
-> success: None
b) PodmanClient(connection='local')
-> success
c) PodmanClient(connection='local', identity='')
-> success
d) PodmanClient(connection='production')
-> success

The explicit identity= argument was ignored because the default fallback str(connection.identity) was evaluated eagerly, raising a TypeError before the fallback was even used. This fix lazily computes the fallback.

Fixes: #652
Fixes: #653
Signed-off-by: Pratham patidarpratham40@gmail.com

#654 (from_env ignoring CONTAINER_CONNECTION) is a separate fix and depends on this one; happy to follow up.

Honny1

Thanks, I have some comments.

Honny1 · 2026-09-23T17:01:03Z

+            identity = kwargs.get("identity") or connection.identity
+            if identity:
+                api_kwargs["identity"] = str(identity)
+            elif "identity" in api_kwargs:
+                del api_kwargs["identity"]


Well, I see this part as uplicate with rows 81-85. Maybe put that into a separate function?

Honny1 · 2026-09-23T17:01:43Z

@@ -54,10 +54,12 @@ def url(self):

    @cached_property
    def identity(self):


Suggested change

def identity(self):

def identity(self) -> Optional[Path]:

Honny1

Also linter complains:

 F841 Local variable `client` is assigned to but never used
     --> podman/tests/unit/test_podmanclient.py:101:60
      |
   99 |     def test_connect_no_identity(self, mock_api_init, mock_api_close):
  100 |         with mock.patch.multiple(Path, open=self.mocked_open, exists=MagicMock(return_value=True)):
  101 |             with PodmanClient(connection="no_identity") as client:
      |                                                            ^^^^^^
  102 |                 mock_api_init.assert_called_once()
  103 |                 kwargs = mock_api_init.call_args[1]
      |
  help: Remove assignment to unused variable `client`
  
  F841 Local variable `client` is assigned to but never used
     --> podman/tests/unit/test_podmanclient.py:111:84
      |
  109 |     def test_connect_explicit_identity(self, mock_api_init, mock_api_close):
  110 |         with mock.patch.multiple(Path, open=self.mocked_open, exists=MagicMock(return_value=True)):
  111 |             with PodmanClient(connection="no_identity", identity="/custom/key") as client:
      |                                                                                    ^^^^^^
  112 |                 mock_api_init.assert_called_once()
  113 |                 kwargs = mock_api_init.call_args[1]
      |
  help: Remove assignment to unused variable `client`
  
  F841 Local variable `client` is assigned to but never used
     --> podman/tests/unit/test_podmanclient.py:120:81
      |
  118 |     def test_connect_explicit_identity_override(self, mock_api_init, mock_api_close):
  119 |         with mock.patch.multiple(Path, open=self.mocked_open, exists=MagicMock(return_value=True)):
  120 |             with PodmanClient(connection="testing", identity="/custom/key2") as client:
      |                                                                                 ^^^^^^
  121 |                 mock_api_init.assert_called_once()
  122 |                 kwargs = mock_api_init.call_args[1]
      |
  help: Remove assignment to unused variable `client`
  
  F841 Local variable `client` is assigned to but never used
     --> podman/tests/unit/test_podmanclient.py:136:36
      |
  135 |         with mock.patch('podman.client.PodmanConfig', return_value=mock_config):
  136 |             with PodmanClient() as client:
      |                                    ^^^^^^
  137 |                 mock_api_init.assert_called_once()
  138 |                 kwargs = mock_api_init.call_args[1]
      |
  help: Remove assignment to unused variable `client`
  
  E501 Line too long (147 > 100)
     --> podman/tests/unit/test_podmanclient.py:154:101
      |
  152 | … / "podman.sock")
  153 | …
  154 | …l(), expected.replace('%5C', '\\') if '\\' in client.api.base_url.geturl() else expected)
      |                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  155 | …
  156 | …or a user
      |
  
  Found 5 errors.
  No fixes available (4 hidden fixes can be enabled with the `--unsafe-fixes` option).
  
  ruff format..............................................................Failed

Honny1

Can you please sqoush commits and move unrealted changes to separate commit. Thanks!

Honny1 · 2026-09-25T15:49:23Z

 unittest:
 	coverage run -m unittest discover -s podman/tests/unit
-	coverage report -m --skip-covered --fail-under=80 --omit=./podman/tests/* --omit=.tox/* --omit=/usr/lib/*
+	coverage report -m --skip-covered --fail-under=85 --omit=./podman/tests/* --omit=.tox/* --omit=/usr/lib/*


I think those changes are unrelated to what is fixed. Please put them into a separate commit.

Honny1 · 2026-09-25T15:50:23Z

I have enabled CI. Please check if it will pass.

Honny1

I didnt read last chnges fully but. Linter is complaining:

  +++ b/podman/tests/unit/test_podmanclient.py
  @@ -101,7 +101,9 @@ class PodmanClientTestCase(unittest.TestCase):
               with PodmanClient(connection="no_identity"):
                   mock_api_init.assert_called_once()
                   kwargs = mock_api_init.call_args[1]
  -                self.assertEqual(kwargs["base_url"], "ssh://root@localhost:22/run/podman/podman.sock")
  +                self.assertEqual(
  +                    kwargs["base_url"], "ssh://root@localhost:22/run/podman/podman.sock"
  +                )
                   self.assertNotIn("identity", kwargs)
   
       @mock.patch('podman.client.APIClient.close')
  @@ -136,7 +138,9 @@ class PodmanClientTestCase(unittest.TestCase):
               with PodmanClient():
                   mock_api_init.assert_called_once()
                   kwargs = mock_api_init.call_args[1]
  -                self.assertEqual(kwargs["base_url"], "http+ssh://root@localhost:22/run/podman/podman.sock")
  +                self.assertEqual(
  +                    kwargs["base_url"], "http+ssh://root@localhost:22/run/podman/podman.sock"
  +                )
                   self.assertNotIn("identity", kwargs)
   
       def test_connect_404(self):
  Error: Process completed with exit code 1.

Honny1

Please link fixed issues in the commit msg with Fixes: #NUMBER_OF_ISSUE. Thanks!

Before the fix: a) PodmanConfig().services['local'].identity -> failed: TypeError expected str, bytes or os.PathLike object, not NoneType b) PodmanClient(connection='local') -> failed: TypeError expected str, bytes or os.PathLike object, not NoneType c) PodmanClient(connection='local', identity='<dummy>') -> failed: TypeError expected str, bytes or os.PathLike object, not NoneType d) PodmanClient(connection='production') -> success After the fix: a) PodmanConfig().services['local'].identity -> success: None b) PodmanClient(connection='local') -> success c) PodmanClient(connection='local', identity='<dummy>') -> success d) PodmanClient(connection='production') -> success The explicit `identity=` argument was ignored because the default fallback `str(connection.identity)` was evaluated eagerly, raising a TypeError before the fallback was even used. This fix lazily computes the fallback. Fixes: containers#652 Fixes: containers#653 Signed-off-by: Pratham <patidarpratham40@gmail.com> containers#654 (from_env ignoring CONTAINER_CONNECTION) is a separate fix and depends on this one; happy to follow up.

pratham8499 · 2026-10-05T18:18:43Z

@Honny1 I have addressed all the feedback and force-pushed the changes:

Fixed the unused variable and line-length linter errors.
Removed the unrelated Makefile changes.
Added the Fixes: #652 and Fixes: #653 tags to the commit message.
Could you please approve the workflows to run the CI again? Also, the testing-farm checks that failed previously seem unrelated to these changes. Thanks

Honny1 · 2026-10-06T09:30:58Z

I think this line from commit msg: https://github.com/containers/podman-py/issues/654 (from_env ignoring CONTAINER_CONNECTION) is a separate fix and depends on this one; happy to follow up. Is not needed.

Honny1 reviewed Sep 23, 2026

View reviewed changes

pratham8499 deployed to build September 23, 2026 17:02 — with GitHub Actions Active

Honny1 requested changes Sep 24, 2026

View reviewed changes

pratham8499 force-pushed the fix/optional-connection-identity branch from 2ef5d3a to 3bdeb20 Compare September 24, 2026 23:16

Honny1 requested changes Sep 25, 2026

View reviewed changes

pratham8499 deployed to build September 25, 2026 15:50 — with GitHub Actions Active

pratham8499 force-pushed the fix/optional-connection-identity branch from 3bdeb20 to ddc39bf Compare September 26, 2026 08:28

pratham8499 deployed to build September 28, 2026 10:06 — with GitHub Actions Active

Honny1 requested changes Sep 29, 2026

View reviewed changes

pratham8499 force-pushed the fix/optional-connection-identity branch from c09b9fd to 5ab97c1 Compare September 30, 2026 08:45

pratham8499 deployed to build September 30, 2026 09:39 — with GitHub Actions Active

Honny1 requested changes Sep 30, 2026

View reviewed changes

pratham8499 force-pushed the fix/optional-connection-identity branch from 5ab97c1 to faa9707 Compare September 30, 2026 12:52

Benzene071106 mentioned this pull request Oct 1, 2026

ServiceConnection.identity raises TypeError when a connection has no identity #652

Open

pratham8499 deployed to build October 6, 2026 09:29 — with GitHub Actions Active

rjmcgee53192 mentioned this pull request Oct 8, 2026

Honour CONTAINER_CONNECTION in PodmanClient.from_env() #673

Open

Navigation Menu

Repository navigation

fix: make service connection identity optional - #656

fix: make service connection identity optional#656
pratham8499 wants to merge 1 commit into
containers:mainfrom
pratham8499:fix/optional-connection-identity

pratham8499 commented Sep 21, 2026

Uh oh!

Honny1 left a comment

Uh oh!

Uh oh!

Honny1 Sep 23, 2026

Uh oh!

Honny1 Sep 23, 2026

Uh oh!

Honny1 left a comment

Uh oh!

Honny1 left a comment

Uh oh!

Honny1 Sep 25, 2026

Uh oh!

Honny1 commented Sep 25, 2026

Uh oh!

Honny1 left a comment

Uh oh!

Honny1 left a comment

Uh oh!

pratham8499 commented Oct 5, 2026

Uh oh!

Honny1 commented Oct 6, 2026

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -54,10 +54,12 @@ def url(self):

		@cached_property
		def identity(self):

Conversation

pratham8499 commented Sep 21, 2026

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Honny1 Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

Honny1 Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

Honny1 Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

Honny1 commented Sep 25, 2026

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

pratham8499 commented Oct 5, 2026

Uh oh!

Honny1 commented Oct 6, 2026

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants