[6.16.z] feat: Add methods to manage autosign entries for puppet/openvox on a SmartProxy

[Satellite-QE]

Cherrypick of PR: #1376

Added two methods to SmartProxy to manage autosign entries on the puppetserver.
The methods allow to add and delete autosign entries.

This is an implementation of https://apidocs.theforeman.org/foreman/3.16/apidoc/v2/autosign.html for POST and DELETE requests.

Example:

host = target_sat.api.Host().search(query={'name': some_host_name})[0]
host.puppet_ca_proxy.add_autosign_entry(host.name)
host.puppet_ca_proxy.delete_autosign_entry(host.name)

[sourcery-ai]

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `nailgun/entities.py:7789-7796` </location>
<code_context>
+            self._server_config,
+        )
+
+    def delete_autosign_entry(self, certname, **kwargs):
+        """Delete an entry from the puppetserver's autosign file.
+
+        :param certname: Name of the host to be deleted from the autosign file
+        """
+        kwargs = kwargs.copy()
+        kwargs.update(self._server_config.get_client_kwargs())
+        path = f'{self.path()}/autosign/{certname}'
+        return _handle_response(
+            client.delete(path, **kwargs),
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Consider URL encoding certname in the path.

URL-encoding certname will prevent path errors if it contains characters like spaces or slashes.

Suggested implementation:

```python
        from urllib.parse import quote
        encoded_certname = quote(certname, safe='')
        path = f'{self.path()}/autosign/{encoded_certname}'

```

If `urllib.parse.quote` is not already imported elsewhere in the file, you should add:
```python
from urllib.parse import quote
```
at the top of the file with the other imports.
</issue_to_address>

### Comment 2
<location> `tests/test_entities.py:3446-3455` </location>
<code_context>
                 if 'environment' in param:
                     self.assertIn('/environments', post.call_args[0][0])

+    def test_add_autosign_entry(self):
+        """Call :meth:`nailgun.entities.SmartProxy.add_autosign_entry`.
+
+        Assert that
+        * correct fqdn is sent,
+        * proper path is built.
+        """
+        certname = "host.example.com"
+        with self.subTest():
+            with mock.patch.object(client, 'post') as post:
+                self.smart_proxy.add_autosign_entry(certname)
+                self.assertEqual(post.call_count, 1)
+                self.assertIn(f'{self.smart_proxy.path()}/autosign', post.call_args[0][0])
+                self.assertEqual(len(post.call_args[1]), 1)
</code_context>

<issue_to_address>
**suggestion (testing):** Consider adding tests for error handling and invalid input for add_autosign_entry.

Please add tests for invalid certname values and for client.post exceptions or error responses to improve coverage of error handling.

Suggested implementation:

```python
    def test_add_autosign_entry(self):
        """Call :meth:`nailgun.entities.SmartProxy.add_autosign_entry`.

        Assert that
        * correct fqdn is sent,
        * proper path is built.
        """
        certname = "host.example.com"
        with self.subTest():
            with mock.patch.object(client, 'post') as post:
                self.smart_proxy.add_autosign_entry(certname)
                self.assertEqual(post.call_count, 1)
                self.assertIn(f'{self.smart_proxy.path()}/autosign', post.call_args[0][0])
                self.assertEqual(len(post.call_args[1]), 1)
                self.assertEqual(post.call_args[1], {'data': {'id': 'host.example.com'}})

    def test_add_autosign_entry_invalid_certname(self):
        """Test add_autosign_entry with invalid certname values."""
        invalid_certnames = ["", None, "invalid fqdn!"]
        for certname in invalid_certnames:
            with self.subTest(certname=certname):
                with mock.patch.object(client, 'post') as post:
                    with self.assertRaises(ValueError):
                        self.smart_proxy.add_autosign_entry(certname)
                    post.assert_not_called()

    def test_add_autosign_entry_post_exception(self):
        """Test add_autosign_entry when client.post raises an exception."""
        certname = "host.example.com"
        with mock.patch.object(client, 'post', side_effect=Exception("API error")) as post:
            with self.assertRaises(Exception) as exc:
                self.smart_proxy.add_autosign_entry(certname)
            self.assertIn("API error", str(exc.exception))
            self.assertEqual(post.call_count, 1)

    def test_add_autosign_entry_post_error_response(self):
        """Test add_autosign_entry when client.post returns an error response."""
        certname = "host.example.com"
        mock_response = mock.Mock()
        mock_response.status_code = 400
        mock_response.json.return_value = {"error": "Invalid certname"}
        with mock.patch.object(client, 'post', return_value=mock_response) as post:
            with self.assertRaises(Exception):
                self.smart_proxy.add_autosign_entry(certname)
            self.assertEqual(post.call_count, 1)

```

These tests assume that `add_autosign_entry` raises a `ValueError` for invalid certnames and raises an `Exception` for error responses or exceptions from `client.post`. If the actual implementation uses different exception types or error handling, you may need to adjust the exception classes accordingly.
</issue_to_address>

### Comment 3
<location> `tests/test_entities.py:3462-3471` </location>
<code_context>
+                self.assertEqual(len(post.call_args[1]), 1)
+                self.assertEqual(post.call_args[1], {'data': {'id': 'host.example.com'}})
+
+    def test_delete_autosign_entry(self):
+        """Call :meth:`nailgun.entities.SmartProxy.add_autosign_entry`.
+
+        Assert that
+        * correct fqdn is sent,
+        * proper path is built.
+        """
+        certname = "host.example.com"
+        with self.subTest():
+            with mock.patch.object(client, 'delete') as delete:
+                self.smart_proxy.delete_autosign_entry(certname)
+                self.assertEqual(delete.call_count, 1)
+                self.assertIn(
+                    f'{self.smart_proxy.path()}/autosign/{certname}', delete.call_args[0][0]
+                )
+                self.assertEqual(len(delete.call_args[1]), 0)
+

</code_context>

<issue_to_address>
**suggestion (testing):** Add tests for error scenarios and invalid input for delete_autosign_entry.

Please add tests for deleting non-existent entries, invalid certnames, and for handling exceptions or error responses from client.delete to ensure comprehensive coverage.

Suggested implementation:

```python
    def test_delete_autosign_entry(self):
        """Call :meth:`nailgun.entities.SmartProxy.delete_autosign_entry`.

        Assert that
        * correct fqdn is sent,
        * proper path is built.
        """
        certname = "host.example.com"
        with self.subTest():
            with mock.patch.object(client, 'delete') as delete:
                self.smart_proxy.delete_autosign_entry(certname)
                self.assertEqual(delete.call_count, 1)
                self.assertIn(
                    f'{self.smart_proxy.path()}/autosign/{certname}', delete.call_args[0][0]
                )
                self.assertEqual(len(delete.call_args[1]), 0)

    def test_delete_autosign_entry_nonexistent(self):
        """Test deleting a non-existent autosign entry returns error or handles gracefully."""
        certname = "nonexistent.example.com"
        with self.subTest():
            with mock.patch.object(client, 'delete', side_effect=Exception("Not found")) as delete:
                with self.assertRaises(Exception) as exc:
                    self.smart_proxy.delete_autosign_entry(certname)
                self.assertIn("Not found", str(exc.exception))
                self.assertEqual(delete.call_count, 1)

    def test_delete_autosign_entry_invalid_certname(self):
        """Test deleting with invalid certnames."""
        invalid_certnames = ["", None, "bad/certname", "invalid!@#"]
        for certname in invalid_certnames:
            with self.subTest(certname=certname):
                with mock.patch.object(client, 'delete') as delete:
                    with self.assertRaises((ValueError, TypeError, Exception)):
                        self.smart_proxy.delete_autosign_entry(certname)
                    # Optionally, check that delete is not called for obviously invalid input
                    # self.assertEqual(delete.call_count, 0)

    def test_delete_autosign_entry_client_error(self):
        """Test handling of client.delete raising an exception."""
        certname = "host.example.com"
        with self.subTest():
            with mock.patch.object(client, 'delete', side_effect=Exception("Client error")) as delete:
                with self.assertRaises(Exception) as exc:
                    self.smart_proxy.delete_autosign_entry(certname)
                self.assertIn("Client error", str(exc.exception))
                self.assertEqual(delete.call_count, 1)

```

- If `delete_autosign_entry` does not currently raise exceptions for invalid certnames, you may need to add input validation to that method in the implementation.
- Adjust the exception types in the invalid certname test if your implementation raises specific exceptions.
- If your client.delete returns error responses instead of raising exceptions, you may need to mock return values and check for error handling logic.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}