-
Notifications
You must be signed in to change notification settings - Fork 133
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
Catch PermissionError when checking for dependencies #433
base: master
Are you sure you want to change the base?
Conversation
If a tool or a directory in the PATH environment variable doesn't have the right permissions, it will trigger a PermissionError and cause ofrak to crash while testing for dependencies. Instead, we should catch the error and issue a warning.
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.
This looks ready to merge to me
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.
On second thought, needs tests.
Adding test case for catching PermissionError when searching for tools in PATH
Added a test to |
found = [r for r in caplog.records if r.message == expected] | ||
assert len(found) == 1, "Missing error message from component_model" |
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.
found = [r for r in caplog.records if r.message == expected] | |
assert len(found) == 1, "Missing error message from component_model" | |
assert any(r.message == expected for r in caplog.records), "Missing error message from component_model" |
This way of doing it is a little more concise. Will probably need to re-run the linter after applying the suggestion, though.
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.
On a second read, my suggested change is not exactly the same as what the original test does, so feel free to resolve this without accepting the edit.
But this particular test is failing in the CI check, so this will have to be revised in some way before we can merge.
Catch PermissionError when checking for dependencies, and issue a warning.
Fixes this issue