Hi Willian,
Thanks for addressing my comments, there are other minor suggestions
inline, but nothing special.
Review approved.
> === modified file 'configs/testrunner/launchpad-lazr.conf'
> --- configs/testrunner/launchpad-lazr.conf 2009-07-29 18:53:51 +0000
> +++ configs/testrunner/launchpad-lazr.conf 2009-08-20 13:09:20 +0000
> @@ -195,3 +195,6 @@
>
> [vhosts]
> use_https: False
> +
> +[zeca]
> +root: /var/tmp/zeca.test
>
Good change, I forgot to mention it before. Tests won't mess with you
production setup.
> === modified file 'lib/canonical/zeca/ftests/harness.py'
> --- lib/canonical/zeca/ftests/harness.py 2009-06-25 05:30:52 +0000
> +++ lib/canonical/zeca/ftests/harness.py 2009-08-20 13:09:20 +0000
> @@ -41,6 +41,8 @@
> ...
> Results for Key 0xDFD20543
> ...
> + pub 1024D/DFD20543 2005-04-13 Sample Person (revoked) <>
> + ...
>
> A key content lookup form via GET.
>
> @@ -52,6 +54,41 @@
> ...
> Results for Key 0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543
> ...
> + -----BEGIN PGP PUBLIC KEY BLOCK-----
> + Version: GnuPG v1.4.9 (GNU/Linux)
> +
> + mQGiBEJdmOcRBADkNJPTBuCIefBdRAhvWyD9SSVHh8GHQWS7l9sRLEsirQkKz1yB
> + ...
> +
> + We can also request a key ID instead of a fingerprint, and it will glob
> + for the fingerprint.
> +
> + >>> print urlopen(
> + ... '%s/pks/lookup?op=get&'
> + ... 'search=0xDFD20543' % root_url
> + ... ).read()
> +
> + ...
> + Results for Key 0xDFD20543
> + ...
> + -----BEGIN PGP PUBLIC KEY BLOCK-----
> + Version: GnuPG v1.4.9 (GNU/Linux)
> +
> + mQGiBEJdmOcRBADkNJPTBuCIefBdRAhvWyD9SSVHh8GHQWS7l9sRLEsirQkKz1yB
> + ...
> +
> + If we request a nonexistent key, we get a nice error.
> +
> + >>> print urlopen(
> + ... '%s/pks/lookup?op=get&'
> + ... 'search=0xDFD20544' % root_url
> + ... ).read()
> +
> + ...
> + Results for Key 0xDFD20544
> + ...
> + Key Not Found
> + ...
>
> A key submit form via POST (see doc/gpghandler.txt for more information).
>
Cool! functional tests documenting key-lookups using keyids.
> === added file 'lib/canonical/zeca/ftests/test_locate_key.py'
> --- lib/canonical/zeca/ftests/test_locate_key.py 1970-01-01 00:00:00 +0000
> +++ lib/canonical/zeca/ftests/test_locate_key.py 2009-08-20 13:09:20 +0000
> @@ -0,0 +1,43 @@
> +# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +import unittest
> +import os.path
> +
> +from canonical.zeca.zeca import locate_key
> +
> +
> +class LocateKeyTestCase(unittest.TestCase):
> + root = os.path.join(os.path.dirname(__file__), 'keys')
> +
> + def assertKeyFile(self, suffix, filename):
> + """Verify that a suffix maps to the given filename."""
> +
> + if filename is not None:
> + filename = os.path.join(self.root, filename)
> + self.assertEqual(locate_key(self.root, suffix), filename)
> +
> + def test_exact_fingerprint_match(self):
> + self.assertKeyFile(
> + '0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get',
> + '0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get')
> +
> + def test_keyid_glob_match(self):
> + self.assertKeyFile(
> + '0xDFD20543.get',
> + '0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get')
> +
> + def test_keyid_without_prefix_glob_match(self):
> + self.assertKeyFile(
> + 'DFD20543.get',
> + '0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get')
> +
> + def test_keyid_no_match(self):
> + self.assertKeyFile('0xDEADBEEF.get', None)
> +
> +
Here, for consistency with our code guideline, you should name your
test cases as: 'test_locate_key_'
I'm slightly worried about the multiple terms included in the variable
you call 'suffix' ('.'), but I guess
changing it would require a deeper refactoring. Better do it later
when we have time.
> +def test_suite():
> + suite = unittest.TestSuite()
> + suite.addTest(unittest.makeSuite(LocateKeyTestCase))
> + return suite
> +
One simpler way of loading all unittest at one would be:
{{{
def test_suite():
return unittest.TestLoader().loadTestsFromName(__name__)
}}}
Since you have only one TestCase class, it's not a big deal, It's your
choice, but consider it new changes.
> === modified file 'lib/canonical/zeca/zeca.py'
> --- lib/canonical/zeca/zeca.py 2009-06-25 05:30:52 +0000
> +++ lib/canonical/zeca/zeca.py 2009-08-20 13:09:20 +0000
> @@ -9,12 +9,13 @@
>
> - 'index' : returns key index information
> - 'get': returns an ASCII armored public key
> -
> -It does not depend on GPG; it simply serves the information stored in
> -files at a given HOME (default to /home/keys/) with the following name
> -format:
> -
> -0x.
> + - 'add': adds a key to the collection (does not update the index)
> +
> +It only depends on GPG for key submission; for retrieval and searching
> +it just looks for files in the root (eg. /var/tmp/zeca). The files
> +are named like this:
> +
> +0x.
>
Thanks for updating this docstring.
> Example:
>
> @@ -35,6 +36,7 @@
> 'Zeca',
> ]
>
> +import glob
> import os
> import cgi
>
> @@ -50,6 +52,33 @@
> GREETING = 'Copyright 2004-2008 Canonical Ltd.\n'
Can you please update this copyright to '2004-2009 ...' ?
> +def locate_key(root, suffix):
> + """Find a key file in the root with the given suffix.
> +
> + This does some globbing to possibly find a fingerprint-named key
> + file when given a key ID.
> +
> + :param root: The root directory in which to look.
> + :param suffix: The key ID or fingerprint, of the form
> + 0x.
> + :returns: An absolute path to the key file.
> + """
> + path = os.path.join(root, suffix)
> +
> + if not os.path.exists(path):
> + # GPG might request a key ID from us, but we name the keys by
> + # fingerprint. Let's glob.
> + if suffix.startswith('0x'):
> + suffix = suffix[2:]
> + keys = glob.glob(os.path.join(root, '*'+suffix))
> + if len(keys) == 1:
> + path = keys[0]
> + else:
> + return None
> +
> + return path
> +
> +
Right, the 'suffix' name is mildly confusing, but the code does what
it is supposed to do. Maybe in another time we can make it clear and
completely isolate the filesystem-based key lookup.
It's already a win to allow keys to be found via keyid, one step at
time, right ?
> class Zeca(Resource):
> def getChild(self, name, request):
> if name == '':
> @@ -95,15 +124,11 @@
>
> filename = '%s.%s' % (keyid, action)
>
> - path = os.path.join(self.root, filename)
> -
> - try:
> - fp = open(path)
> - except IOError:
> + path = locate_key(self.root, filename)
> + if path is not None:
> + content = cgi.escape(open(path).read())
> + else:
> content = 'Key Not Found'
> - else:
> - content = cgi.escape(fp.read())
> - fp.close()
>
> page += '\n%s\n
\n' % content
>
Nice move!
That's it, summing up, if you agree with the changes I suggested:
1. Rename new unittest test cases.
2. Update the visible copyright on zeca home page.
we can land it.
Thanks for adopting this poor little orphan application. I glad to see
it solving problems in the real world.
[]
--
Celso Providelo
IRC: cprov, Jabber: , Skype: cprovidelo
1024D/681B6469 C858 2652 1A6E F6A6 037B B3F7 9FF2 583E 681B 6469