-
Notifications
You must be signed in to change notification settings - Fork 4
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
85 client silently ignores pem file if user expansion is needed #86
85 client silently ignores pem file if user expansion is needed #86
Conversation
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.
Lets add to the docstring for key
that is will be expanding using expanduser()
, so it will be clear what the behavior will be. Can we also add a simple test to confirm the exception is throw?
I've added a few tests for the key file now. |
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.
Looking good, thanks for adding the tests. One inline comment.
temp_path = Path().home() / ".sfapi_test1" | ||
if temp_path.exists(): | ||
(temp_path / "key.pem").unlink(missing_ok=True) | ||
temp_path.rmdir() |
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.
Sorry all the operations that could raise an exception should be in the try block, so like this:
try:
tmp_path_factory._basetemp = Path().home()
key_path = tmp_path_factory.mktemp(".sfapi_test1", numbered=False) / "key.pem"
# Make a fake key for testing
key_path.write_text(
f"""abcdefghijlmo
-----BEGIN RSA PRIVATE KEY-----
{rsa.generate_private_key(public_exponent=65537, key_size=2048)}
-----END RSA PRIVATE KEY-----
"""
)
key_path.chmod(0o100600)
yield key_path
finally:
# make sure to cleanup the test since we put a file in ~/.sfapi_test
temp_path = Path().home() / ".sfapi_test1"
if temp_path.exists():
(temp_path / "key.pem").unlink(missing_ok=True)
temp_path.rmdir()
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 okay makes sense!
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.
LGTM, thanks for making the updates.
Checks to get full path of file and if the file isn't found raises an error.