Update azure_keyvault_backend to turn ServiceRequestError into a RuntimeError#152
Update azure_keyvault_backend to turn ServiceRequestError into a RuntimeError#152webknjaz merged 4 commits intoansible:develfrom
azure_keyvault_backend to turn ServiceRequestError into a RuntimeError#152Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds handling for Azure SDK network/DNS and generic Azure errors in the Azure Key Vault backend by converting them to chained RuntimeError; updates tests to mock Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PabloHiro
force-pushed
the
fix/azure-tests
branch
from
4727ceb to
3a5225f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/azure_kv_test.py`:
- Around line 67-80: The test test_azure_kv_dns_error currently triggers a real
DNS lookup; instead, patch the Azure SecretClient used by azure_keyvault_backend
so its get_secret method raises azure.core.exceptions.ServiceRequestError to
simulate a DNS/network failure. Use the same mocking pattern as
test_azure_kv_invalid_env to inject the exception, call
azure_keyvault_backend(...) and assert it raises RuntimeError with the 'Failed
to connect to Azure Key Vault' message; reference SecretClient.get_secret and
azure_keyvault_backend when locating the code to change.
🧹 Nitpick comments (1)
tests/azure_kv_test.py (1)
39-46: Redundant mock setup: line 39 is dead code and line 46 double-patches the same attribute.
mocker.patch.object(azure_kv, 'SecretClient')(line 40) already replacesazure_kv.SecretClientfor the test's duration. Themocker.Mock()on line 39 is immediately discarded, and themonkeypatch.setattron line 46 redundantly re-patches the same attribute.Suggested cleanup
- mock_client = mocker.Mock() - mock_client = mocker.patch.object(azure_kv, 'SecretClient') + mock_client = mocker.patch.object(azure_kv, 'SecretClient') mock_client.return_value.get_secret.side_effect = ( CredentialUnavailableError( message='ManagedIdentityCredential authentication unavailable.', ) ) - monkeypatch.setattr(azure_kv, 'SecretClient', mock_client)
PabloHiro
changed the title
PabloHiro
force-pushed
the
fix/azure-tests
branch
from
3a5225f to
d066735
Compare
PabloHiro
force-pushed
the
fix/azure-tests
branch
from
d066735 to
f4dccd3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/azure_kv_test.py (1)
68-81:⚠️ Potential issue | 🟠 MajorThis test still relies on real DNS resolution — contradicts PR objectives.
test_azure_kv_dns_errormakes an unmocked call to the Azure SDK, depending on DNS resolution forkeyvault.testto fail withServiceRequestError. In CI environments with DNS interceptors, captive portals, or restricted networking, the failure mode may differ (timeout, different exception type), making this test flaky.The PR objectives explicitly require mocking network calls. Mock
SecretClient.get_secretto raiseServiceRequestErrordirectly, consistent with the pattern used in the other tests:Suggested fix
-def test_azure_kv_dns_error() -> None: +def test_azure_kv_dns_error( + mocker: MockerFixture, +) -> None: """Test DNS resolution error is converted to RuntimeError.""" + from azure.core.exceptions import ServiceRequestError + + mock_client = mocker.patch.object(azure_kv, 'SecretClient', autospec=True) + mock_client.return_value.get_secret.side_effect = ServiceRequestError( + message='Name resolution failed for keyvault.test', + ) + with pytest.raises( RuntimeError, match=r'^Failed to connect to Azure Key Vault: .', ): azure_kv.azure_keyvault_backend( url='https://keyvault.test', client='client-id', secret='client-secret', tenant='tenant-id', secret_field='secret', secret_version='', )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/azure_kv_test.py` around lines 68 - 81, The test test_azure_kv_dns_error currently performs a real network/DNS call; change it to mock Azure's SecretClient.get_secret to raise azure.core.exceptions.ServiceRequestError so the azure_keyvault_backend(url=..., client=..., secret=..., tenant=..., secret_field=..., secret_version='') path is exercised deterministically and converted to a RuntimeError; use the same mocking pattern as other tests (patch SecretClient.get_secret to side_effect=ServiceRequestError(...)) and keep the pytest.raises(RuntimeError, match=...) assertion unchanged.
🧹 Nitpick comments (1)
tests/azure_kv_test.py (1)
40-47: Dead assignment and redundant double-patching.Line 40 (
mocker.Mock()) is immediately overwritten on line 41 — it's dead code. Additionally,mocker.patch.objecton line 41 already replacesazure_kv.SecretClientfor the test's lifetime, so themonkeypatch.setattron line 47 is redundant. The same pattern is repeated intest_azure_kv_generic_azure_error(lines 89–90, 94).Suggested cleanup
- mock_client = mocker.Mock() - mock_client = mocker.patch.object(azure_kv, 'SecretClient', autospec=True) + mock_client = mocker.patch.object(azure_kv, 'SecretClient', autospec=True) mock_client.return_value.get_secret.side_effect = ( CredentialUnavailableError( message='ManagedIdentityCredential authentication unavailable.', ) ) - monkeypatch.setattr(azure_kv, 'SecretClient', mock_client)Apply the same simplification to
test_azure_kv_generic_azure_error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/azure_kv_test.py` around lines 40 - 47, The test creates a dead assignment and double-patches azure_kv.SecretClient: remove the initial mocker.Mock() assignment and drop the redundant monkeypatch.setattr call; instead rely solely on mocker.patch.object(azure_kv, 'SecretClient', autospec=True) and configure its return_value.get_secret.side_effect (used in tests like test_azure_kv_generic_azure_error and the other failing test) to raise CredentialUnavailableError or the generic error as appropriate; apply this same cleanup to both tests mentioned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/azure_kv_test.py`:
- Around line 68-81: The test test_azure_kv_dns_error currently performs a real
network/DNS call; change it to mock Azure's SecretClient.get_secret to raise
azure.core.exceptions.ServiceRequestError so the azure_keyvault_backend(url=...,
client=..., secret=..., tenant=..., secret_field=..., secret_version='') path is
exercised deterministically and converted to a RuntimeError; use the same
mocking pattern as other tests (patch SecretClient.get_secret to
side_effect=ServiceRequestError(...)) and keep the pytest.raises(RuntimeError,
match=...) assertion unchanged.
---
Nitpick comments:
In `@tests/azure_kv_test.py`:
- Around line 40-47: The test creates a dead assignment and double-patches
azure_kv.SecretClient: remove the initial mocker.Mock() assignment and drop the
redundant monkeypatch.setattr call; instead rely solely on
mocker.patch.object(azure_kv, 'SecretClient', autospec=True) and configure its
return_value.get_secret.side_effect (used in tests like
test_azure_kv_generic_azure_error and the other failing test) to raise
CredentialUnavailableError or the generic error as appropriate; apply this same
cleanup to both tests mentioned.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
src/awx_plugins/credentials/azure_kv.pytests/azure_kv_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/awx_plugins/credentials/azure_kv.py
PabloHiro
force-pushed
the
fix/azure-tests
branch
from
f4dccd3 to
a5f60c1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/azure_kv_test.py (1)
69-82:⚠️ Potential issue | 🟠 Major
test_azure_kv_dns_errorrelies on real DNS — flaky in CI.The test makes a live network call and depends on the OS returning an error for
keyvault.test. DNS interceptors, captive portals, or corporate resolvers can cause a different failure mode (timeout, different exception type), breaking the assertion. The PR objectives and the linked issue explicitly require mocking instead of real DNS.🐛 Proposed fix — mock SecretClient instead of relying on DNS
-def test_azure_kv_dns_error() -> None: +def test_azure_kv_dns_error(mocker: MockerFixture) -> None: """Test DNS resolution error is converted to RuntimeError.""" + from azure.core.exceptions import ServiceRequestError + + mock_client = mocker.patch.object(azure_kv, 'SecretClient', autospec=True) + mock_client.return_value.get_secret.side_effect = ServiceRequestError( + message='Name resolution failed for keyvault.test', + ) + with pytest.raises( RuntimeError, match=r'^Failed to connect to Azure Key Vault: .', ): azure_kv.azure_keyvault_backend( url='https://keyvault.test', client='client-id', secret='client-secret', tenant='tenant-id', secret_field='secret', secret_version='', )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/azure_kv_test.py` around lines 69 - 82, The test test_azure_kv_dns_error calls azure_kv.azure_keyvault_backend and currently performs a real network/DNS resolution; instead patch the Azure SecretClient used by that backend so instantiating SecretClient raises a DNS-like exception (e.g., OSError or ConnectionError) and verify azure_keyvault_backend converts it to the RuntimeError with message starting "Failed to connect to Azure Key Vault: ". Use a mocking facility (monkeypatch or unittest.mock.patch) to replace azure.keyvault.secrets.SecretClient (or the symbol imported/used inside the azure_kv module) to raise the exception on construction, then call azure_kv.azure_keyvault_backend in the test and assert the RuntimeError as before.src/awx_plugins/credentials/azure_kv.py (1)
164-167:⚠️ Potential issue | 🟠 MajorMissing
from catchall_azure_errorbreaks the exception chain.The
AzureErrorhandler silently drops the original exception cause. Compare with theServiceRequestErrorhandler two lines above that correctly chains viafrom request_err. This was explicitly requested in the prior review and acknowledged as "Done!" but was not applied.🐛 Proposed fix
except _az_exc.AzureError as catchall_azure_error: raise RuntimeError( f'Error retrieving secret from Azure Key Vault: {catchall_azure_error}', - ) + ) from catchall_azure_error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/awx_plugins/credentials/azure_kv.py` around lines 164 - 167, The AzureError except block in azure_kv.py currently raises a RuntimeError without chaining the original exception; update the except _az_exc.AzureError as catchall_azure_error handler to re-raise the RuntimeError using "from catchall_azure_error" so the original cause is preserved (mirror the pattern used in the ServiceRequestError handler/request_err). Ensure the RuntimeError message remains descriptive while adding the exception chaining.
🧹 Nitpick comments (1)
tests/azure_kv_test.py (1)
40-47: Dead assignment and redundantmonkeypatch.setattr— same pattern repeated at lines 90-95.
mocker.Mock()on line 40 is immediately overwritten on line 41 (dead code).mocker.patch.objectat line 41 already replacesazure_kv.SecretClientwith the mock, so themonkeypatch.setattrat line 47 is a no-op (identical pattern repeated intest_azure_kv_generic_azure_errorat lines 90-95).♻️ Proposed cleanup (apply same fix at lines 90-95)
- mock_client = mocker.Mock() mock_client = mocker.patch.object(azure_kv, 'SecretClient', autospec=True) mock_client.return_value.get_secret.side_effect = ( CredentialUnavailableError( message='ManagedIdentityCredential authentication unavailable.', ) ) - monkeypatch.setattr(azure_kv, 'SecretClient', mock_client)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/azure_kv_test.py` around lines 40 - 47, Remove the dead assignment and redundant monkeypatch by deleting the initial mocker.Mock() and the monkeypatch.setattr call; use only mocker.patch.object(azure_kv, 'SecretClient', autospec=True) to patch azure_kv.SecretClient and then set mock_client.return_value.get_secret.side_effect as currently written; apply the same cleanup to the other test case (test_azure_kv_generic_azure_error) that repeats this pattern so both tests only use mocker.patch.object and no extra monkeypatch or overwritten mocker.Mock().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/awx_plugins/credentials/azure_kv.py`:
- Around line 164-167: The AzureError except block in azure_kv.py currently
raises a RuntimeError without chaining the original exception; update the except
_az_exc.AzureError as catchall_azure_error handler to re-raise the RuntimeError
using "from catchall_azure_error" so the original cause is preserved (mirror the
pattern used in the ServiceRequestError handler/request_err). Ensure the
RuntimeError message remains descriptive while adding the exception chaining.
In `@tests/azure_kv_test.py`:
- Around line 69-82: The test test_azure_kv_dns_error calls
azure_kv.azure_keyvault_backend and currently performs a real network/DNS
resolution; instead patch the Azure SecretClient used by that backend so
instantiating SecretClient raises a DNS-like exception (e.g., OSError or
ConnectionError) and verify azure_keyvault_backend converts it to the
RuntimeError with message starting "Failed to connect to Azure Key Vault: ". Use
a mocking facility (monkeypatch or unittest.mock.patch) to replace
azure.keyvault.secrets.SecretClient (or the symbol imported/used inside the
azure_kv module) to raise the exception on construction, then call
azure_kv.azure_keyvault_backend in the test and assert the RuntimeError as
before.
---
Nitpick comments:
In `@tests/azure_kv_test.py`:
- Around line 40-47: Remove the dead assignment and redundant monkeypatch by
deleting the initial mocker.Mock() and the monkeypatch.setattr call; use only
mocker.patch.object(azure_kv, 'SecretClient', autospec=True) to patch
azure_kv.SecretClient and then set
mock_client.return_value.get_secret.side_effect as currently written; apply the
same cleanup to the other test case (test_azure_kv_generic_azure_error) that
repeats this pattern so both tests only use mocker.patch.object and no extra
monkeypatch or overwritten mocker.Mock().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
src/awx_plugins/credentials/azure_kv.pytests/azure_kv_test.py
PabloHiro
force-pushed
the
fix/azure-tests
branch
from
a5f60c1 to
b7a0f70
Compare
|
With the pylint env workaround in bfcb034, I'm rebasing this since that should've cured the linting job. |
webknjaz
force-pushed
the
fix/azure-tests
branch
from
b7a0f70 to
068173f
Compare
webknjaz
changed the title
azure_keyvault_backend to turn ServiceRequestError into a RuntimeError
|
@PabloHiro could you rebase this? |
webknjaz
left a comment
webknjaz
left a comment
There was a problem hiding this comment.
Alright.. Let's get this in! It just needs a rebase + signed commits.
…eError Assisted-by: Claude
…imeError Assisted-by: Claude
PabloHiro
force-pushed
the
fix/azure-tests
branch
from
068173f to
79dbffb
Compare
@webknjaz done, please merge at will |
webknjaz
left a comment
webknjaz
left a comment
There was a problem hiding this comment.
@PabloHiro I'll need to re-approve once you fix that last linting violation.
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
github-project-automation
Bot
moved this from 🧐 @webknjaz's review queue 📋
to ⏰ Merge reminder needed 🔀
in 📅 Procrastinating in public 😵💫
2 participants
Closes: #146
Summary by CodeRabbit
Bug Fixes
Tests