-
Notifications
You must be signed in to change notification settings - Fork 296
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
Added a try/except for importing pam with logging an error and update… #151
Conversation
…d README to note that when using pam you need to install pam module
On a successful upload with pam
|
Test failure looks like it may have been non-deterministic. Re-running. |
LOL. I was so very confused when I was looking at the test and the results disappeared from my screen. |
The beauty of distributed testing 🔮 |
Looks like it may be a real problem |
o.O how does this one fail testing where the last one didn't. Looks like bad request got returned when trying to upload. I tested it by hand and didn't see any issues.
EDIT: Noticed it was failing for bdist and bdist_wheel.
Still running fine. Might be something in the test env (mine or travis). |
I’ll try running them again.
|
Hey, there we go. |
3rd time IS the charm then. |
return pam.authenticate(username, password) | ||
except ImportError as error: | ||
log.error('PAM module not found. Please install pam module') | ||
return False | ||
|
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 :-(
We don't try/catch
for missing dependencies - the original exception is quite explanatory to the user (in combination with the documentation, which is the one needing improvement).
Huh. I didn't know that. Want to pull it out? |
Yes, lets keep the code under diet :-) thanks! [edit:] To be consistent, we would have to do the same also for |
I'll have a PR up for that shortly |
See #152 |
@mplanchard please wait.
why don't we keep everything to the documentation, and leave the new code for a future release, when updating the |
You mean the |
Commented in #152 what i suggest. |
Added a try/except for importing pam with logging an error and update…
…d README to note that when using pam you need to install pam module