Code review comment for lp:~javier.collado/utah/sshmixin_destroy_fixes

Revision history for this message
Max Brustkern (nuclearbob) wrote :

When I originally implemented the exception-raising destroy in Machine, I wasn't thinking that destroy would be something we'd need much inheritance on. I think moving that to an empty implementation in Machine (or one that just returns False) might be a good way to go.

As far as the exceptions go, I didn't really take inheritance into account well when setting up all those methods. Perhaps a clearer exception would be something like "{class} has called the {method} method of the base Machine class, which is not implemented."

I think changing the inheritance order is going to cause problems with all those exception-creating base methods. We can remove those, but if we do that, I'd like to see some way of reporting that we've called a method that a Machine does not implement that's more readable than the default python exception. Maybe we just need to be catching that and raising something else.

Finally, a note on the config file. Anything that uses a directory or information on the system on which the package is installed currently needs to go through a special routine to prevent the config info from being written when the package is built. It's this function that does that:
def dumpdefaults(build=False):
    if build:
        del DEFAULTS['bridge']
        del DEFAULTS['logfile']
        del DEFAULTS['sshprivatekey']
        del DEFAULTS['sshpublickey']
        del DEFAULTS['user']
        del DEFAULTS['vmpath']
    return json.dumps(DEFAULTS, sort_keys=True, indent=4)
The reason I do that is so that we can ship a config file that's based on config.py, but still goes in /etc/utah and takes advantage of the debian packaging facilities for tracking user changes to config files. This may not be the best approach in the long run. If you've got ideas on that, we can discuss them. For now I'm going to make the system-specific options a separate dict which gets added the defaults, and then we can remove it if we need to.

review: Approve

« Back to merge proposal