Code review comment for lp:~hazmat/pyjuju/ensemble-resolved

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

Excerpts from Gustavo Niemeyer's message of Tue Apr 26 19:30:57 UTC 2011:
> Review: Needs Fixing
> Looks good. Mostly just points we already talked about, plus moving
> some of the error checking logic into the state API itself.
>
>
> [1]
>
> + sub_parser = subparsers.add_parser("resolved", help=command.__doc__)
>
> The nice docs are in the resolved function.

Moved the long docs to subcommand description for resolved -h, updated the
command doc with a better summary.

>
> [2]
>
> + service_name, _ = unit_name.split("/")
> + service_state = yield service_manager.get_service_state(service_name)
> + unit_state = yield service_state.get_unit_state(unit_name)
>
> Please invert the ordering here so that unit_state is retrieved
> first. This will automatically validate the unit_name value
> provided by the user, and avoid an ugly traceback in case the
> user provides a bad unit_name value.
>

done.

[3-6]

as per irc discussions, moving the validation logic and internal-ids to inside
set-relation-resolved is eing deferred to another bug. The unit agent
implementation of resolved will need to verify values as well before acting
upon a unit/relation.

« Back to merge proposal