Code review comment for lp:~bcsaller/pyjuju/statusd

> [1]
>
> The hook up on the provisioning agent seems to be completely untested.
> Someone could mistakingly remove the line and everything would be
> apparently fine.

Added some tests to verify the watch fires w/o paying much attention
to the results as we don't build a proper environment to status in
those tests. Also had to mock StatusManager out for the expose tests
so as not to interfere with its watch tests.

>
> [2]
>
> +def get_active_machine_provider(client):
>
> This would benefit from some testing too.

Added. Verify we can get a dummy provider back when configured for such.

>
> [3]
>
> +        exists = yield self._client.exists("/status")
> +        if not exists:
> +            returnValue(None)
> +
> +        ystatus, _ = yield self._client.get("/status")
> +        status = yaml.load(ystatus)
>
> Race condition and unnecessary roundtrip here. Get and test for errors,
> rather than testing and then getting.
>

Thank, based on this point I cleaned up the get and set paths and am
using retry_change in the sets. The gets ask forgiveness rather than
permission (conceptually speaking).

> [4]
>
> +        if status is not None and "version" not in status or
> status["version"] != VERSION:
> +            raise EnsembleError("Version mismatch in status")
>
> Crashing like that means we won't ever be able to change the version
> number once a first version was used. Instead of crashing, pretend
> it didn't exist if the version was wrong. Please test this.
>

[4,5,6]

Any error in recovering state or parsing version should now force a
new status collection phase.