Code review comment for lp:~jimbaker/pyjuju/charm-format-2

Reviewers: mp+108831_code.launchpad.net,

Message:
Please take a look.

Description:
Adds format: 2 support to juju, while maintaining backwards
compatibility for old charms.

Overview
========

This merge proposal adds support for a new key to a charm's
``metadata.yaml`` file, ``format`` such that charms can choose to use
a more consistent formatting (YAML) when working with relation and
configuration settings, without a backwards breaking change. Another
important part of this change is consistency: YAML is used throughout,
which works well with the existing YAML storage of relation/config
settings in ZK itself.

Example metadata
================

Here's an example metadata file for format: 2 charms:

name: wordpress
summary: "WordPress is a full featured web blogging tool"
maintainer: Clint Byrum <email address hidden>
description: |
  WordPress is a full featured web blogging tool:
  - Instant publishing (no rebuilding)
  - Comment pingback support with spam protection
  - Non-crufty URLs
  - Themable
  - Plugin support
requires:
   db:
     interface: mysql
provides:
   website:
     interface: http
format: 2

format: 1
=========

When using format: 1 (this format is implied when the format key is
undefined in ``metadata.yaml``), Juju for this charm maintains
existing format support for specifying configuration and relation
settings and for outputting relation settings when using the smart
format for ``relation-get``.

When using ASCII strings to define scalar values, this is not an issue
for working with relation settings. For config settings, because
everything is a string, it's not possible to actually set boolean
config settings (regardless of the capitalization):

$ juju set wordpress good=True
2012-06-05 14:44:35,893 INFO Connecting to environment...
2012-06-05 14:44:38,890 INFO Connected to environment.
Invalid value for good: 'True'
2012-06-05 14:44:39,108 ERROR Invalid value for good: 'True'

When a map is output (``relation-get -``), smart format continues to
use the ``__str__`` formatting, in this case of the internally used
Python dict. In this, we also see the impact of how parsing all values
as strings (in relation-set/set), then using JSON for encoding
transport converts all strings to Unicode. This results in output like
the following:

{u"public-address": u"ec2-1-2-3-4.compute-1.amazonaws.com",
  u"configured": u"True",
  u"foo": u"bar"}

Per the discussion on the mailing list, this exposes Python
implementation details and needs to be fixed, especially in light of
the Go port.

format: 2
=========

When the format is set to 2, relation (``relation-set``) and config
(``juju set``) settings are parsed as YAML. When using
``relation-get`` with smart format, output is in YAML, using the same
YAML formatting options as ``juju get`` for consistency/human
readibility.

Output for v2 format is as follows for scalars, each on a separate line:

true # Input was true (or True or TRUE, YAML is especially tolerant
of its input)
false
A string

Maps are output like so. A bash script likely could even readily
written to parse this output, without the use of a helper:

configured: true
foo: bar
public-address: ec2-1-2-3-4.compute-1.amazonaws.com

It is possible for charms of different charm format to be deployed in
an environment. Besides unit testing, this scenario has been
extensively tested when deployed.

New environment variable: CHARM_FORMAT
======================================

For relation-set/relation-get, the new environment variable
``CHARM_FORMAT``, as set for the relation hook context, controls
parsing and output. This can also be used in debug hooks to see the
impact of choosing one format versus the other.

Implementation notes
====================

Overall code changes are small, with most changes seen in tests. In
addition, tests were added to describe the current observed format: 1
behavior.

Two parallel code paths were implemented in impacted code. If it's
desired to modify the behavior of format: 2, this can be readily done
on that code path.

yaml.safe_load and yaml.safe_dump are used to ensure that Python types
do not leak and to avoid introducing a security hole. The use of YAML
also allows for consistent support of Unicode strings (not using ASCII
codepoints) as well as binary data (as introduced via the !!binary
tag). Unlike the format: 1 implementation, the consistent use of YAML
for encoding/decoding ensures that there is not an unexpected encoding
issue that's encountered, such as seen in bug #901495.

https://code.launchpad.net/~jimbaker/juju/charm-format-2/+merge/108831

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6308044/

Affected files:
   A [revision details]
   M juju/charm/config.py
   M juju/charm/metadata.py
   A juju/charm/tests/repository/series/mysql-format-v2/config.yaml
   A juju/charm/tests/repository/series/mysql-format-v2/metadata.yaml
   A juju/charm/tests/repository/series/mysql-format-v2/revision
   A juju/charm/tests/repository/series/mysql/config.yaml
   M juju/charm/tests/repository/series/mysql/metadata.yaml
   M juju/charm/tests/test_metadata.py
   M juju/control/config_set.py
   M juju/control/tests/test_config_set.py
   M juju/hooks/cli.py
   M juju/hooks/invoker.py
   M juju/hooks/protocol.py
   M juju/hooks/tests/test_cli.py
   M juju/hooks/tests/test_communications.py
   M juju/hooks/tests/test_invoker.py
   M juju/state/tests/test_relation.py