Merge lp:~ddstreet/launchpadlib/nosudo into lp:launchpadlib
Status: | Merged | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Merged at revision: | 178 | ||||||||||||
Proposed branch: | lp:~ddstreet/launchpadlib/nosudo | ||||||||||||
Merge into: | lp:launchpadlib | ||||||||||||
Diff against target: |
322 lines (+171/-44) 2 files modified
src/launchpadlib/credentials.py (+151/-43) src/launchpadlib/launchpad.py (+20/-1) |
||||||||||||
To merge this branch: | bzr merge lp:~ddstreet/launchpadlib/nosudo | ||||||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson | 2020-02-22 | Approve on 2020-04-14 | |
Review via email:
|
- 178. By Dan Streetman on 2020-02-22
-
Do not use keyring or browser under sudo
- 179. By Dan Streetman on 2020-03-13
-
Fixes based on MR feedback
Dan Streetman (ddstreet) wrote : | # |
- 180. By Dan Streetman on 2020-03-30
-
Fallback to MemoryCredentia
lStore if NoKeyringError Instead of passing NoKeyringError up to calling application, change
default behavior to fallback to MemoryCredentialStore. LP: #1864204
Dan Streetman (ddstreet) wrote : | # |
I meant to create a new branch with the last commit for a separate MR, but I don't know the bzr cmdline well enough so it seems I accidentally pushed it to this branch. I'll leave it in this MR since it does follow on the previous commits for this but if you would prefer I can try to move it into a separate MR.
Dan Streetman (ddstreet) wrote : | # |
Also, it would be *super awesome* if launchpadlib could be moved into a git repo instead of bzr... ;-)
Colin Watson (cjwatson) wrote : | # |
Looks mostly good, but tox reports that this fails tests on Python 2.7 and 3.5 like this:
Failure in test test_credential
Traceback (most recent call last):
File "/usr/lib/
testMethod()
File "/home/
self.
File "/usr/lib/
assertion_
File "/usr/lib/
raise self.failureExc
AssertionError: 0 != 1
Failure in test test_default_
Traceback (most recent call last):
File "/usr/lib/
testMethod()
File "/home/
launchpadli
File "/usr/lib/
callableObj
File "/usr/lib/
"{0} not raised"
AssertionError: RuntimeError not raised
This is because 2.7 and 3.5 are no longer supported by the version of keyring that has your patch to add the NoKeyringError exception, so tox picks an older version of keyring: in other words, this is indicating a slight incompatibility with the older keyring version.
Things seem to work well if I apply this patch on top:
=== modified file 'src/launchpadl
--- old/src/
+++ new/src/
@@ -393,7 +393,12 @@ class KeyringCredenti
try:
- except NoKeyringError:
+ except NoKeyringError as e:
+ # keyring < 21.2.0 raises RuntimeError rather than anything more
+ # specific. Make sure it's the exception we're interested in.
+ if (NoKeyringError == RuntimeError and
+ 'No recommended backend was available' not in str(e)):
+ raise
if self._fallback:
else:
@@ -405,7 +410,12 @@ class KeyringCredenti
try:
- except NoKeyringError:
+ except NoKeyringError as e:
+ # keyring < 21.2.0 raises RuntimeError rather than anything more
+ # specific. Make sure it's the exception we're interested in.
+ if (NoKeyringError == Runtime...
- 181. By Dan Streetman on 2020-04-13
-
Apply cjwatson patch to handle older Keyring code that may throw RuntimeError to indicate real error, instead of NoKeyringError
Dan Streetman (ddstreet) wrote : | # |
Yep, I see what you're saying; I can't really think of any better way to handle older Keyring code that would throw RuntimeError to indicate a real error. I applied your patch, and should now be pushed here. Thanks!
updated based on your feedback; let me know if you have any more feedback with the updated code.
Sorry if I am not getting the bzr commits correct, I use git and have almost no experience with bzr :(