Code review comment for lp:~bcsaller/pyjuju/config-set

Very nicely organized, thanks. Just a few details before we can merge this:

[1]

+ """Set Service Options.

s/Service/service/
s/Options/options/

[2]

+ Services provide various options which can be set using this
+ command.

Here is a slightly more comprehensive version:

  Service formulas may define dynamic options which may be tweaked
  at deployment time, or over the lifetime of the service. This
  command allows changing these settings.

[3]

The overall logic in the command looks good.

Some minor details:

+ formula_manager = FormulaStateManager(client)
+ formula = yield formula_manager.get_formula_by_service_name(service_name)
+
+ service_manager = ServiceStateManager(client)
+ service = yield service_manager.get_service_state(service_name)

The function get_formula_by_service_name is untested and is likely
not necessary. E.g.:

    formula = yield service.get_formula_state()

[4]

+ # Parse the options using the formula config.yaml metadata to
+ # validate
+ # This will raise (and must be caught) on error
+ # parse config options

This comment would benefit from some additional love.

[5]

+ except KeyError:
+ # its ok not to contain a config.yaml
+ # it means no options will be handled
+ pass

KeyError can mean lots of things in Python. It'd be nice if rather than
catching this exception the logic would check for the existence of the
file in advance. This way we'll know we're never ignoring a KeyError
which we didn't intend to.

[6]

+def parse_keyvalue_pairs(options):

This needs tests.

[7]

+ @classmethod
+ def load_file(cls, pathname, optional=False):
(...)
+ def load(self, data):

Why is 'load_file' a class method and 'load' a normal one?

[8]

+ if not data or not isinstance(data, dict):
+ raise ValueError("Expected YAML dict of options metadata")

We need a more formal definition and verification of the config.yaml schema,
similar to how we do it for the metadata file.

[9]

+ raise ValueError(
+ "Expected `options` top level key in options metadata")
(...)
+ raise KeyError(
+ "%s is not a valid configuration option." % (
+ option))

We should never raise KeyError or ValueError or any other standard Python
exceptions, or we'll have no way to catch the *application* error without
wondering if we're not catching something we didn't intend to by mistake.

[10]

+ # Setup the watch deferred callback after the user defined callback
+ # has returned successfully from the existence invocation.
+ callback_d = maybeDeferred(callback, bool(exists))
+ callback_d.addCallback(
+ lambda x: watch_d.addCallback(watcher) and x)

This will need some tweaks along the lines of the work Kapil has been
pushing forward. Please catch up with him about it.