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

On Mon, May 23, 2011 at 2:37 PM, Gustavo Niemeyer <email address hidden> wrote:
> Review: Needs Fixing
>
> Very nicely organized, thanks.  Just a few details before we can merge this:
>
>
> [1]
>
> +    """Set Service Options.
>
> s/Service/service/
> s/Options/options/
>
>
Done

> [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.
>

Changed.

> [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()
>

Changed and removed formula_by_service_name.

> [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.
Cleaned
>
> [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.

Changed to ServiceConfigError throughout.

>
> [6]
>
> +def parse_keyvalue_pairs(options):
>
> This needs tests.

It had coverage in terms of the cli test which exercised the arg
parsing but a specific test has been added as well.

>
> [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?
>

The thinking was that load_file was just a convenience method wrapping
the common usage of init and load calls but I agree the distinction
isn't very clear.

> [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.

I included a proposed version of this. It out of the scope of the spec
we agreed to but does reflect some of our talks about possible
directions for this.

>
> [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.
>

These are all ServiceConfigError now. If we need to subclass that down
the road we can, but seems to provide enough granularity for our use
case.

> [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.

I think I'm doing the correct thing here, but I still need to talk
with Kapil about it. We should maybe have a doc on the current best
practice around this so we don't introduce regressions down the line.

> --
> https://code.launchpad.net/~bcsaller/ensemble/config-set/+merge/60235
> You are the owner of lp:~bcsaller/ensemble/config-set.
>