Hi!
On Mon, Jul 21, 2014 at 3:52 PM, Klaus Aehlig <aeh...@google.com> wrote:
> > +def _AddPublicKeyProcessLine(new_uuid, new_key, line_uuid, line_key,
> tmp_file,
> > + uuid_found):
>
> While being an internal function, please still add documentation.
> Having 6 positional arguments is not a function "obvious" to understand
> just by its name. Same for other internal functions. Or even better, add
> a general documentation of the way ManipulatePubKeyFile calls the functions
> passed in; the documentation there is too generic.
>
Okay.
>
> > + if line_uuid == new_uuid and line_key == new_key:
> > + logging.debug("SSH key of node '%s' already in key file.", new_uuid)
> > + uuid_found = True
> > + else:
> > + tmp_file.write("%s %s" % (line_uuid, line_key))
> > + return uuid_found
> > +
> > +
> > +def _AddPublicKeyElse(new_uuid, new_key, tmp_file):
> > + tmp_file.write("%s %s\n" % (new_uuid, new_key))
> > +
> > +
> > +def _RemovePublicKeyProcessLine(
> > + target_uuid, target_key, # pylint: disable=W0613
>
> Instead of disabling the pylint warning, choose a name starting with _ for
> variables that are not used. Also applies to other parts of this patch.
>
Sure, did not know about that "trick".
>
> > + line_uuid, line_key, tmp_file, uuid_found):
> > + if line_uuid != target_uuid:
> > + tmp_file.write("%s %s" % (line_uuid, line_key))
> > + return uuid_found
>
> This function always passes through the value of uuid_found.
> In particular, in RemovePublicKey the "else"-function _RemovePublicKeyElse
> is always called which does not seem to fit with the log message added
> there.
>
True, I figured out a bug in the control flow here. It did not show up yet,
because the else branch is essentially only traversed in recovery
situations. I'll resend the whole patch with the correct flow.
Cheers,
Helga
>
> > +
> > +
> > +def _RemovePublicKeyElse(
> > + new_uuid, new_key, tmp_file): # pylint: disable=W0613
> > + logging.debug("Trying to remove key of node '%s' which is not in list"
> > + " of public keys.", new_uuid)
> > +
> > +
> > +def _ReplaceNameByUuidProcessLine(
> > + node_name, key, # pylint: disable=W0613
> > + line_identifier, line_key, tmp_file, uuid_found, node_uuid=None):
> > + if node_name == line_identifier:
> > + uuid_found = True
> > + tmp_file.write("%s %s" % (node_uuid, line_key))
> > + else:
> > + tmp_file.write("%s %s" % (line_identifier, line_key))
> > + return uuid_found
> > +
> > +
> > +def _ReplaceNameByUuidElse(
> > + node_uuid, node_name, key, tmp_file): # pylint: disable=W0613
> > + logging.debug("Trying to replace node name '%s' with UUID '%s', but"
> > + " no line with that name was found.", node_name,
> node_uuid)
> > +
> > +
> > +def ManipulatePubKeyFile(target_identifier, target_key,
> > + key_file=pathutils.SSH_PUB_KEYS,
> > + error_fn=errors.ProgrammerError,
> > + process_line_fn=None, process_else_fn=None):
> > + """Manipulates the list of public SSH keys of the cluster.
> > +
> > + @type target_identifier: str
> > + @param target_identifier: identifier of the node whose key is added;
> in most
> > + cases this is the node's UUID, but in some it is the node's host
> name
> > + @type target_key: str
> > + @param target_key: string containing a public SSH key (a complete line
> > + possibly including more parameters than just the key)
> > + @type key_file: str
> > + @param key_file: filename of the file of public node keys (optional
> > + parameter for testing)
> > + @type error_fn: function
> > + @param error_fn: Function that returns an exception, used to customize
> > + exception types depending on the calling context
> > + @type process_line_fn: function
> > + @param process_line_fn: function to process one line of the public
> key file
> > + @type process_else_fn: function
> > + @param process_else_fn: function to be called if no line of the key
> file
> > + matches the target uuid
> > +
> > + """
> > + assert process_else_fn is not None
> > + assert process_line_fn is not None
>
> [...]
>
> --
> Klaus Aehlig
> Google Germany GmbH, Dienerstr. 12, 80331 Muenchen
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschaeftsfuehrer: Graham Law, Christine Elizabeth Flores
>
--
Helga Velroyen | Software Engineer | hel...@google.com |
Google Germany GmbH
Dienerstr. 12
80331 München
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores
