Code review comment for lp:~leonardr/launchpadlib/separate-login-for-cronjob

The new CredentialStore stuff is cool.

I've got lots of comments, but they're all fairly minor.

[1]

+ * Launchpad.login_securely, for programs that are run with user
+ interaction.
+ * Launchpad.login_insecurely, for programs that must run with no
+ user interaction.

These names worry me. I don't want to debate these names if they have
already been discussed. I just want to say: if I am the only person to
have seen them then I think they need some discussion, though not a
lot.

[2]

+
+
+ def do_load(self, unique_key):

Dangerous extra blank line.

[3]

Some lint:

credentials.py:29: 'base64' imported but unused
credentials.py:34: 'sys' imported but unused
credentials.py:35: 'textwrap' imported but unused
credentials.py:37: 'quote' imported but unused
launchpad.py:25: 'socket' imported but unused
launchpad.py:26: 'stat' imported but unused
launchpad.py:29: 'HostedFile' imported but unused
launchpad.py:29: 'ScalarValue' imported but unused
launchpad.py:36: 'SystemWideConsumer' imported but unused
launchpad.py:52: 'STAGING_SERVICE_ROOT' imported but unused
launchpad.py:52: 'EDGE_SERVICE_ROOT' imported but unused
testing/helpers.py:37: 'simplejson' imported but unused
testing/helpers.py:41: 'AuthorizeRequestTokenWithBrowser' imported but unused
testing/helpers.py:41: 'Credentials' imported but unused
tests/test_launchpad.py:21: 'StringIO' imported but unused
tests/test_launchpad.py:27: 'textwrap' imported but unused
tests/test_launchpad.py:226: local variable 'launchpad' is assigned to but never used
tests/test_launchpad.py:253: local variable 'launchpad' is assigned to but never used
tests/test_launchpad.py:263: local variable 'launchpad' is assigned to but never used
tests/test_launchpad.py:320: local variable 'launchpad' is assigned to but never used
tests/test_launchpad.py:534: local variable 'launchpad' is assigned to but never used
tests/test_launchpad.py:618: local variable 'service_root' is assigned to but never used

[4]

+ def __init__(self, file, credential_save_failed=None):
+ super(UnencryptedFileCredentialStore, self).__init__(
+ credential_save_failed)
+ self.file = file

Because file() is a builtin function/type consider using filename or
filepath instead, or something like that.

[5]

+ This method is deprecated as of launchpadlib version
+ 1.9.0. You should use Launchpad.login_anonymously() for
+ anonymous access, Launchpad.login_insecurely() for scripts
+ that need to run without any user interaction, and
+ Launchpad.login_securely() for all other purposes, possibly
+ specifying a custom authorization_engine or credential_store.

Perhaps it's worth putting some or all of this into a deprecation
warning, warnings.warn("...", DeprecationWarning)?

[6]

+@contextmanager
+def fake_keyring(fake):
+ original_keyring = launchpadlib.credentials.keyring
+ launchpadlib.credentials.keyring = fake
+ yield
+ launchpadlib.credentials.keyring = original_keyring

This will break when a test fails because the yield can raise an
exception. Something like the following would work better I think:

@contextmanager
def fake_keyring(fake):
    original_keyring = launchpadlib.credentials.keyring
    launchpadlib.credentials.keyring = fake
    try:
        yield
    finally:
        launchpadlib.credentials.keyring = original_keyring

[7]

+ self.filename = tempfile.mktemp()

mkstemp() should be used instead of mktemp().

The docstring says it's "unsafe and should not be used". Why it's
still in the standard library I don't really know!

[8]

+ def test_filename(self):
+ filename = tempfile.mktemp()

Again, mkstemp().

[9]

+ launchpad = NoNetworkLaunchpad.login_insecurely(
+ filename, application_name='not important')
+
+ # The credentials are stored unencrypted in the file you
+ # specify.
+ credentials = Credentials.load_from_path(filename)
+ self.assertEquals(credentials.consumer.key,
+ launchpad.credentials.consumer.key)
+ os.remove(filename)

This won't get run if there's an earlier exception.

If you're happy to make launchpadlib depend on testtools, and inherit
from testtools.TestCase, you could use self.addCleanup(os.remove,
filename) immediately after calling mkstemp() to avoid this (small)
problem.

[10]

+ def test_credential_save_failed(self):
+ # Verify that credential_save_failed is called if there's a
+ # problem writing the file.
+ #stub_file = StringIO()
+ #launchpad = NoNetworkLaunchpad.login_insecurely(
+ # stub_file, application_name='not important')
+ pass

Did you mean to do something with this?

[11]

+ def test_login_with_with_credentials_file(self):
+ # If you pass credentials_file into login_with, the
+ # credentials are stored in that file.
+ filename = tempfile.mktemp()

Another mktemp().

[12]

+ launchpad = NoNetworkLaunchpad.login_with(
+ 'not important', credentials_file=filename)
+ self.assertTrue(
+ isinstance(launchpad.credential_store,
+ UnencryptedFileCredentialStore))
+ os.remove(filename)

Same as [9].