Code review comment for lp:~ev/uci-engine/requires-ready-deployment

On Mon, Nov 17, 2014 at 08:21:04PM -0000, Evan Dandrea wrote:
> Evan Dandrea has proposed merging lp:~ev/uci-engine/requires-ready-deployment into lp:uci-engine.
>
> Requested reviews:
> Canonical CI Engineering (canonical-ci-engineering)
>
> For more details, see:
> https://code.launchpad.net/~ev/uci-engine/requires-ready-deployment/+merge/242023
>
> Require a ready deployment before running the integration tests.
>

See my comments in-line. Nothing to halt landing this MP just food for
thought.

[snip]
> +class ReadyDeployment(features.Feature):
> + def _probe(self):

I think it was Ned Batchelder that did a talk where he admonished using
a class with a single method in it and asked "why not just use a
method?"

> + print 'here we go'

I think this can be removed, right?

> + try:
> + out = subprocess.check_output(['juju', 'status'])
> + except subprocess.CalledProcessError:
> + return False

We should probably log something here.

> +
> + status = yaml.safe_load(out)
> + for service, service_data in status['services'].iteritems():
> + for unit, unit_data in service_data['units'].iteritems():
> + agent_state = unit_data['agent-state']
> + if agent_state in ('pending', 'error'):
> + msg = '{} in {} state.'.format(unit, agent_state)
> + print >>sys.stderr, msg

I seem to recall this method of writing to stderr being frowned upon.
For python3 compatibility you can do something like:

  from future import print_function

  print(msg, file=sys.stderr)

[snip]