[MERGE] Authentication ring implementation (read-only)

Martin Pool mbp at canonical.com
Sat Nov 3 22:31:43 GMT 2007


Martin Pool has voted tweak.
Status is now: Conditionally approved
Comment:
I apologize for the delay in merging this.  I think the long diff put me
off, though it's not really hard to read.

+   * New ``authentication.conf`` file to define remote servers 
credentials.
+     (Vincent Ladeuil)

I'd change that to

   ... file holding the password or other credentials for remote servers.
   This can be used for ssh, sftp, smtp and other supported transports.

-            warning('BZREMAIL is deprecated in favor of BZR_EMAIL. 
Please update your configuration.')
+            # FIXME: Seems to have been deprecated since more than a 
year now,
+            # time to delete ? -- vila 20071019
+            trace.warning('BZREMAIL is deprecated in favor of 
BZR_EMAIL.'
+                          ' Please update your configuration.')

Please go ahead and give one.

+class AuthenticationConfig(object):

I was surprised this is not inheriting from one of the existing
config classes, but I don't really see a reason for it to do so.

+        super(AuthenticationConfig, self).__init__()

If it doesn't actually *have* a super method, it seems a bit strange or
misleading to call it.  I don't know if we have a specific policy on it.
Calling super always to get "always called first" behaviour like Java or
C++ seems not quite right.


+            self._get_filename = authentication_config_filename

It looks like it's assigning a filename, but it's actually assigning a
callable.  Adding comments on the member variables would help.

Actually, is there any reason not to just evaluate it when the object's
constructed?

+            self._input = self._get_filename()
+        else:
+            self._get_filename = None
+            self._input = _file


+                a_port = None
+            except ValueError:
+                raise ValueError("'port' not numeric in %s" % 
auth_def_name)

If these are coming from the user's configuration file, and represent
configuration errors by the user then

  - This should not really be ValueError, because that will give a
    traceback.  It does not need a custom class.
  - Maybe they should be just warnings?
  - Maybe they should include the line number, if that can be easily
    determined?
  - They should give the config file name.

Maybe it would be better to split out the code that matches an entry
against a search query out from the fact it comes from the config for
easier testing.

+            auth = {'host': self._host, 'port': self._port,
+                    'user': user, 'password': password,
+                    'protocol': self._unqualified_scheme,
+                    'path': self._path}

I find these easier to read as

   dict(host=self._host, port=self._port, ..)

etc, when all the keys are Python keywords.  Less quoting.

=== modified file 'doc/developers/authentication-ring.txt'

You need to selectively move some of this into the user documentation, 
now
that it'll be accessible to users.  I think this really should be done
before merge, even if it's just a cut of the specification text (which 
is
already pretty clear.)

Aside from those (particularly the docs) I would be happy to have it
merged.


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3Cm28x5tyj89.fsf%40free.fr%3E



More information about the bazaar mailing list