-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(azure): catch import error as reportable #6714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(azure): catch import error as reportable #6714
Conversation
cjp256
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to capture the cloud-init log for the failure in your message to demonstrate the testing was done successfully.
Can you add a test case in:
- test_errors.py for this new class
- test_azure.py to demonstrate this. maybe look to test_query_vm_id_system_uuid_failure for inspiration
Thanks Cade!!
Thanks for the review @cjp256! Added in tests in those two files respectively. Also, I added the output message from the VM to the commit message above. I'd be happy to make any additional changes as needed. |
cloudinit/sources/DataSourceAzure.py
Outdated
| blowfish_hash = passlib.hash.sha512_crypt.hash | ||
| except ImportError: | ||
| except ImportError as error: | ||
| _import_error = error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to save it to another variable
| _import_error = error |
error=error is fine below
if you don't like the looks, feel free to rename this as import_error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current implementation (26eb9f, the commit below), the exception that gets caught is actually the same issue that is detected via Ruff:
exception=NameError("name ''error'' is not defined")
I believe this to be an issue with the error coming after the blowfish function, which is technically a new scope. I will put together more information and bring it for discussion tomorrow!
|
|
||
|
|
||
| def test_import_error(): | ||
| exception = ImportError("No module named 'foobar'", name="foobar") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want a real error to test against you can go ahead and simulate error with a real import error
try:
import ModuleThatDoesNotExist
except Exception as error:
...
That way it's easier to assume that the ImportError is instantiated like the running environment will.
| def test_import_error_from_failed_import(self): | ||
| """Attempt to import a module that is not present""" | ||
| try: | ||
| import nonexistent_module_that_will_never_exist # type: ignore[import-not-found] # noqa: F401 # isort:skip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see you did that here
| ) | ||
|
|
||
| def test_import_error_from_failed_import(self): | ||
| """Attempt to import a module that is not present""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what we really want here is coverage for encrypt_pass() on the import failure
there's some basic coverage in TestDependencyFallback to ensure it works with one or the other, but lacks a test covering the case where neither exists.
I think the easiest way to test this with a mock is to pull all of that import logic into blowfish_hash() instead of being top-level, and force the import behavior you want with a side effect.
Something like:
import builtins
import pytest
def blowfish_hash(password: str) -> str:
from passlib.hash import sha512_crypt
return sha512_crypt.hash(password)
class TestBlowfishHash:
def test_success(self, monkeypatch):
class FakeSha512Crypt:
@staticmethod
def hash(pw: str) -> str:
return f"fake-sha512:{pw}"
real_import = builtins.__import__
def fake_import(name, globals=None, locals=None, fromlist=(), level=0):
# Intercept: from passlib.hash import sha512_crypt
if name == "passlib.hash" and fromlist and "sha512_crypt" in fromlist:
class FakePasslibHashModule:
sha512_crypt = FakeSha512Crypt
return FakePasslibHashModule()
return real_import(name, globals, locals, fromlist, level)
monkeypatch.setattr(builtins, "__import__", fake_import)
assert blowfish_hash("secret") == "fake-sha512:secret"
def test_importerror(self, monkeypatch):
real_import = builtins.__import__
def fake_import(name, globals=None, locals=None, fromlist=(), level=0):
if name == "passlib.hash" and fromlist and "sha512_crypt" in fromlist:
raise ImportError("passlib is not installed")
return real_import(name, globals, locals, fromlist, level)
monkeypatch.setattr(builtins, "__import__", fake_import)
with pytest.raises(ImportError, match="passlib"):
blowfish_hash("secret")
Proposed Commit Message
Test Steps
python3-passlibuninstalledMerge type
cc: @cjp256 @peytonr18