feat(core): extract url checker and support multiple api endpoints#5
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the KernelCare compatibility check by extracting URL-checking logic into a helper and extending compatibility detection to probe multiple API endpoints.
Changes:
- Extracted URL probing into a new internal helper (
_check_url). - Updated
is_compat()to try multiple endpoints (/version,/latest.v2,/latest.v3) before returning unsupported. - Added unit tests covering
_check_urlbehavior and newis_compat()fallback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
kc-compat.py |
Extracts URL checking and updates compatibility detection to try multiple endpoints. |
test_kc_compat.py |
Adds tests for _check_url and new multi-endpoint fallback behavior in is_compat(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _check_url(url): | ||
| try: | ||
| urlopen(url) | ||
| return True |
There was a problem hiding this comment.
_check_url() calls urlopen(url) but never closes the returned response object and uses the default (potentially unbounded) timeout. This can leak sockets/file descriptors and may cause the CLI to hang indefinitely on slow/stalled connections. Consider opening the response with an explicit timeout and ensuring it is closed (e.g., close in a finally block or use contextlib.closing).
| def test_is_compat_fallback_to_v2(self, mock_urlopen, mock_hash): | ||
| mock_urlopen.side_effect = [ | ||
| HTTPError(None, 404, 'Not Found', None, None), | ||
| MagicMock(), | ||
| ] | ||
| assert kc_compat.is_compat() == True | ||
| assert mock_urlopen.call_count == 2 | ||
| mock_urlopen.assert_called_with(self.BASE + '/latest.v2') | ||
|
|
There was a problem hiding this comment.
This test only asserts the last urlopen() call (assert_called_with) plus call_count; it doesn't verify that is_compat() actually tried the expected sequence (e.g., /version first, then /latest.v2). Using assert_has_calls with the full expected call list would make the fallback behavior harder to regress.
| def test_is_compat_fallback_to_v3(self, mock_urlopen, mock_hash): | ||
| mock_urlopen.side_effect = [ | ||
| HTTPError(None, 404, 'Not Found', None, None), | ||
| HTTPError(None, 404, 'Not Found', None, None), | ||
| MagicMock(), | ||
| ] | ||
| assert kc_compat.is_compat() == True | ||
| assert mock_urlopen.call_count == 3 | ||
| mock_urlopen.assert_called_with(self.BASE + '/latest.v3') |
There was a problem hiding this comment.
Similar to the v2 fallback test, this only asserts the final urlopen() call and total call_count. Consider asserting the full call order (e.g., /version, /latest.v2, /latest.v3) to ensure the fallback chain is actually exercised as intended.
| @patch.object(kc_compat, 'get_kernel_hash', return_value='abcdef123456') | ||
| @patch.object(kc_compat, 'urlopen') | ||
| def test_is_compat_404_error_returns_false(self, mock_urlopen, mock_hash): | ||
| def test_is_compat_all_404_returns_false(self, mock_urlopen, mock_hash): | ||
| mock_urlopen.side_effect = HTTPError(None, 404, 'Not Found', None, None) | ||
| assert kc_compat.is_compat() == False | ||
| assert mock_urlopen.call_count == 3 |
There was a problem hiding this comment.
This asserts call_count==3, but doesn't validate which URLs were attempted. Adding an assert_has_calls for the three expected endpoints would better lock in the intended behavior when all endpoints return 404.
No description provided.