Merge lp:~jimbaker/pyjuju/charm-format-2 into lp:pyjuju

[Jim Baker]

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://codereview.appspot.com/6308044/

[Jim Baker (jimbaker)]

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

[Gustavo Niemeyer (niemeyer)]

I haven't reviewed the code, but the description of the change looks
good.

One detail that deserves some attention is the use of the CHARM_FORMAT
environment variable. If we have it, it should be named
JUJU_CHARM_FORMAT, similar to other variables we have. That said, do we
actually need this variable? This sounds like an implementation detail
that is not worth exposing. relation-get and relation-set both
communicate with the unit, and can readily discover the format based on
that.

https://codereview.appspot.com/6308044/

[Gustavo Niemeyer (niemeyer)]

On further thinking, if the environment variable makes the change
simpler, we may as well just do it. Can we please make this purely an
implementation detail, though: call it _JUJU_CHARM_FORMAT, no docs. How
does that sound?

https://codereview.appspot.com/6308044/

[Jim Baker (jimbaker)]

On 2012/06/06 02:18:58, niemeyer wrote:
> On further thinking, if the environment variable makes the change
simpler, we
> may as well just do it. Can we please make this purely an
implementation detail,
> though: call it _JUJU_CHARM_FORMAT, no docs. How does that sound?

I like this suggestion: _JUJU_CHARM_FORMAT properly suggests this
environment variable is an implementation detail. The use of the env var
does simplify the implementation.

https://codereview.appspot.com/6308044/

[William Reade (fwereade)]

OK, this is a bit of a monster review... sorry :(. I have some
suggestions that could possibly simplify things quite a lot; please let
me know what you think.

https://codereview.appspot.com/6308044/diff/1/juju/charm/metadata.py
File juju/charm/metadata.py (right):

https://codereview.appspot.com/6308044/diff/1/juju/charm/metadata.py#newcode242
juju/charm/metadata.py:242: if self.format not in (1, 2):
(PYTHON_FORMAT, YAML_FORMAT)?

https://codereview.appspot.com/6308044/diff/1/juju/control/config_set.py
File juju/control/config_set.py (right):

https://codereview.appspot.com/6308044/diff/1/juju/control/config_set.py#newcode98
juju/control/config_set.py:98: os.environ["CHARM_FORMAT"] =
str(charm_format)
This feels like a strange way to go about modifying the behaviour of
parse_keyvalue_pairs below...

https://codereview.appspot.com/6308044/diff/1/juju/hooks/cli.py
File juju/hooks/cli.py (right):

https://codereview.appspot.com/6308044/diff/1/juju/hooks/cli.py#newcode17
juju/hooks/cli.py:17: PYTHON_FORMAT, YAML_FORMAT = 1, 2
I feel like these should probably be somewhere else, if only because I
see late imports here and there. Suggestion: they, and
parse_keyvalue_pairs, should go somewhere else together; possibly
juju.charm?

https://codereview.appspot.com/6308044/diff/1/juju/hooks/cli.py#newcode243
juju/hooks/cli.py:243: print >>stream, serialized.rstrip("\n")
Appears to be duplicated in juju/charm/config.py; utility function?

https://codereview.appspot.com/6308044/diff/1/juju/hooks/cli.py#newcode247
juju/hooks/cli.py:247: """Return the format defined by $CHARM_FORMAT"""
Please make this function module-private; IMO this is the only place we
should be getting charm format from the environment (as opposed to...
from a charm ;)).

https://codereview.appspot.com/6308044/diff/1/juju/hooks/cli.py#newcode248
juju/hooks/cli.py:248: env_charm_format = os.environ.get("CHARM_FORMAT")
_JUJU_CHARM_FORMAT

https://codereview.appspot.com/6308044/diff/1/juju/hooks/cli.py#newcode275
juju/hooks/cli.py:275: charm_format = get_charm_format()
charm_format should be a parameter; it's easily accessible to both
clients.

https://codereview.appspot.com/6308044/diff/1/juju/hooks/invoker.py
File juju/hooks/invoker.py (right):

https://codereview.appspot.com/6308044/diff/1/juju/hooks/invoker.py#newcode151
juju/hooks/invoker.py:151: self._charm_format = None
Add an accessor so the UnitAgentServer can get at this value.

https://codereview.appspot.com/6308044/diff/1/juju/hooks/invoker.py#newcode213
juju/hooks/invoker.py:213: CHARM_FORMAT=str(self._charm_format),
+1 on _JUJU_CHARM_FORMAT as suggested elsewhere. (note: it won't be
necessary in the go port.)

https://codereview.appspot.com/6308044/diff/1/juju/hooks/protocol.py
File juju/hooks/protocol.py (right):

https://codereview.appspot.com/6308044/diff/1/juju/hooks/protocol.py#newcode101
juju/hooks/protocol.py:101: ("format", amp.Integer())]
This shouldn't need to be sent up here. The HookInvoker knows what the
value is, and the UnitAgentServer can access it via
self.factory.get_invoker(client_id).

https://codereview.appspot.com/6308044/diff/1/juju/hooks/protocol.py#newcode109
juju/hooks/protocol.py:109: ("format", a...

[William Reade (fwereade)]

Another note: relation-ids and relation-list don't generate yaml; this
is odd, given that we favour yaml everywhere else; and while they should
not *default* to outputting yaml (ie we leave their format_smart methods
alone), we really *should* at least make the option available (by
implementing format_yaml on the base class).

https://codereview.appspot.com/6308044/

[William Reade (fwereade)]

Comments re: internal serialization format withdrawn; they're necessary
to preserve charm format 1 bugginess. Would you comment this explicitly
in somewhere protocol.py?

https://codereview.appspot.com/6308044/

[Jim Baker (jimbaker)]

Please take a look.

https://codereview.appspot.com/6308044/diff/1/juju/charm/metadata.py
File juju/charm/metadata.py (right):

https://codereview.appspot.com/6308044/diff/1/juju/charm/metadata.py#newcode242
juju/charm/metadata.py:242: if self.format not in (1, 2):
On 2012/06/06 13:34:40, fwereade wrote:
> (PYTHON_FORMAT, YAML_FORMAT)?

Done.

https://codereview.appspot.com/6308044/diff/1/juju/control/config_set.py
File juju/control/config_set.py (right):

https://codereview.appspot.com/6308044/diff/1/juju/control/config_set.py#newcode98
juju/control/config_set.py:98: os.environ["CHARM_FORMAT"] =
str(charm_format)
Agreed, just calls the refactored version of parse_keyvalue_pairs that
takes a charm format. Not certain what possessed me to write this code
this way, must have really want to avoid refactoring at that point :)

https://codereview.appspot.com/6308044/diff/1/juju/hooks/cli.py
File juju/hooks/cli.py (right):

https://codereview.appspot.com/6308044/diff/1/juju/hooks/cli.py#newcode17
juju/hooks/cli.py:17: PYTHON_FORMAT, YAML_FORMAT = 1, 2
Moved to juju.lib.format. Putting this in juju.charm would seem to have
too much cross module dependency on what are otherwise independent
modules.

https://codereview.appspot.com/6308044/diff/1/juju/hooks/cli.py#newcode243
juju/hooks/cli.py:243: print >>stream, serialized.rstrip("\n")
Created juju.lib.format and moved this functionality there so it can be
reused as suggested.

https://codereview.appspot.com/6308044/diff/1/juju/hooks/cli.py#newcode247
juju/hooks/cli.py:247: """Return the format defined by $CHARM_FORMAT"""
That won't work, it's also needed by juju.hooks.protocol

https://codereview.appspot.com/6308044/diff/1/juju/hooks/cli.py#newcode248
juju/hooks/cli.py:248: env_charm_format = os.environ.get("CHARM_FORMAT")
On 2012/06/06 13:34:40, fwereade wrote:
> _JUJU_CHARM_FORMAT

Done.

https://codereview.appspot.com/6308044/diff/1/juju/hooks/cli.py#newcode275
juju/hooks/cli.py:275: charm_format = get_charm_format()
On 2012/06/06 13:34:40, fwereade wrote:
> charm_format should be a parameter; it's easily accessible to both
clients.

Done.

https://codereview.appspot.com/6308044/diff/1/juju/hooks/invoker.py
File juju/hooks/invoker.py (right):

https://codereview.appspot.com/6308044/diff/1/juju/hooks/invoker.py#newcode151
juju/hooks/invoker.py:151: self._charm_format = None
On 2012/06/06 13:34:40, fwereade wrote:
> Add an accessor so the UnitAgentServer can get at this value.

Done.

https://codereview.appspot.com/6308044/diff/1/juju/hooks/invoker.py#newcode213
juju/hooks/invoker.py:213: CHARM_FORMAT=str(self._charm_format),
On 2012/06/06 13:34:40, fwereade wrote:
> +1 on _JUJU_CHARM_FORMAT as suggested elsewhere. (note: it won't be
necessary in
> the go port.)

Done.

https://codereview.appspot.com/6308044/diff/1/juju/hooks/protocol.py
File juju/hooks/protocol.py (right):

https://codereview.appspot.com/6308044/diff/1/juju/hooks/protocol.py#newcode101
juju/hooks/protocol.py:101: ("format", amp.Integer())]
On 2012/06/06 13:34:40, fwereade wrote:
> This shouldn't need to be sent up here. The HookInvoker knows what the
value is,
> and the UnitAgentServer can access it via
self.factory.get_invoker(cl...

[lp:~jimbaker/pyjuju/charm-format-2]

559. By Jim Baker on 2012-06-06

Refactor def of PYTHON_FORMAT/YAML_FORMAT constants

560. By Jim Baker on 2012-06-06

Refactored parse_keyvalue_pairs

561. By Jim Baker on 2012-06-06

Use the invoker on the unit agent side of the protocol to retrieve charm_format

562. By Jim Baker on 2012-06-06

Refactored code out of juju.hooks.cli into juju.hooks.utils to avoid circular imports

563. By Jim Baker on 2012-06-06

Refactor to put standard formatting logic in juju.lib.format

564. By Jim Baker on 2012-06-06

Fix failing tests

565. By Jim Baker on 2012-06-07

Refactored format support for parsing/output to be shared across juju

566. By Jim Baker on 2012-06-07

Do not use str.format

567. By Jim Baker on 2012-06-07

PEP8/PyFlakes

568. By Jim Baker on 2012-06-07

Removed debug log stmt

[Jim Baker (jimbaker)]

Comments addressed in the latest version of the proposal.

https://codereview.appspot.com/6308044/

[Jim Baker (jimbaker)]

Yes, it's now there.

https://codereview.appspot.com/6308044/

[Kapil Thangavelu (hazmat)]

http://codereview.appspot.com/6308044/diff/10001/juju/charm/metadata.py
File juju/charm/metadata.py (right):

http://codereview.appspot.com/6308044/diff/10001/juju/charm/metadata.py#newcode239
juju/charm/metadata.py:239: if self.format not in (PYTHON_FORMAT,
YAML_FORMAT):
This seems to be conflating two different things, charm api versions and
there corresponding formats. ie. we could add version 3, with the
existing yaml format. there should be a mapping of format to version.
the charm isn't making a request for format, its making a version api
assertion.

http://codereview.appspot.com/6308044/diff/10001/juju/hooks/protocol.py
File juju/hooks/protocol.py (right):

http://codereview.appspot.com/6308044/diff/10001/juju/hooks/protocol.py#newcode220
juju/hooks/protocol.py:220: if invoker.charm_format == PYTHON_FORMAT:
instead of conditional logic everywhere in this file, this process would
be simpler/cleaner dispatched to a formatter object from lib/format as
an instance property.

http://codereview.appspot.com/6308044/diff/10001/juju/hooks/utils.py
File juju/hooks/utils.py (right):

http://codereview.appspot.com/6308044/diff/10001/juju/hooks/utils.py#newcode10
juju/hooks/utils.py:10: env_charm_format =
os.environ.get("_JUJU_CHARM_FORMAT")
this feels a bit icky, its an immutable property of a charm being passed
as a mutable parameter. the server side could return the format in its
response.

http://codereview.appspot.com/6308044/

[Jim Baker (jimbaker)]

Please take a look.

https://codereview.appspot.com/6308044/

[lp:~jimbaker/pyjuju/charm-format-2]

569. By Jim Baker on 2012-06-14

Initial strategy refactoring

570. By Jim Baker on 2012-06-14

Do not expose YAMLFormat outside of juju.lib.format

571. By Jim Baker on 2012-06-14

Removed YAMLFormat, PythonFormat, etc in favor of using Format2, Forma1

572. By Jim Baker on 2012-06-14

More tests in juju.lib.format

573. By Jim Baker on 2012-06-14

More tests in juju.lib.format

574. By Jim Baker on 2012-06-14

More tests

575. By Jim Baker on 2012-06-15

Yet more tests/docstrings

576. By Jim Baker on 2012-06-15

PyFlakes

577. By Jim Baker on 2012-06-15

Merged trunk

[Jim Baker (jimbaker)]

Please take a look at the latest proposed version of this branch, this
should solve the issues that were raised.

http://codereview.appspot.com/6308044/diff/10001/juju/charm/metadata.py
File juju/charm/metadata.py (right):

http://codereview.appspot.com/6308044/diff/10001/juju/charm/metadata.py#newcode239
juju/charm/metadata.py:239: if self.format not in (PYTHON_FORMAT,
YAML_FORMAT):
On 2012/06/12 21:18:08, hazmat wrote:
> This seems to be conflating two different things, charm api versions
and there
> corresponding formats. ie. we could add version 3, with the existing
yaml
> format. there should be a mapping of format to version. the charm
isn't making a
> request for format, its making a version api assertion.

I changed this so there's a definite distinction, through the use of
Format1 and Format2 classes (no more PYTHON_FORMAT, YAML_FORMAT). Assume
at some point we have a format: 3 and it's supported by the Python code,
then we could readily create a Format3 class that either derives from
Format2, or from BaseFormat, or what makes sense. So this separates the
charm version information (and any assertions) from the implementation.

http://codereview.appspot.com/6308044/diff/10001/juju/hooks/protocol.py
File juju/hooks/protocol.py (right):

http://codereview.appspot.com/6308044/diff/10001/juju/hooks/protocol.py#newcode220
juju/hooks/protocol.py:220: if invoker.charm_format == PYTHON_FORMAT:
The code has been changed such that one can get an appropriate formatter
(Format1, Format2 for now) and do the appropriate action, without the
conditional logic being replicated throughout as you've observed.

http://codereview.appspot.com/6308044/diff/10001/juju/hooks/utils.py
File juju/hooks/utils.py (right):

http://codereview.appspot.com/6308044/diff/10001/juju/hooks/utils.py#newcode10
juju/hooks/utils.py:10: env_charm_format =
os.environ.get("_JUJU_CHARM_FORMAT")
I have changed this usage where feasible to what you describe, however,
in cases like relation_set, it's necessary to do the parsing on the
client side before calling the server side of the implementation. In
this case, it's still necessary to define _JUJU_CHARM_FORMAT. However,
this lookup is now more isolated, because it's been moved to
juju.lib.format.

http://codereview.appspot.com/6308044/

[William Reade (fwereade)]

Damn, I'm sorry, I had an unpublished LGTM sitting here for days :(((.

https://codereview.appspot.com/6308044/diff/11004/juju/hooks/tests/test_invoker.py
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/hooks/tests/test_invoker.py#newcode511
juju/hooks/tests/test_invoker.py:511: @defer.inlineCallbacks
Heh, nice catches :).

https://codereview.appspot.com/6308044/diff/11004/juju/lib/format.py
File juju/lib/format.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/lib/format.py#newcode144
juju/lib/format.py:144: os.environ.get("_JUJU_CHARM_FORMAT", "1")))
Thanks, all this is much clearer.

https://codereview.appspot.com/6308044/

[Kapil Thangavelu (hazmat)]

i'd still like to get rid of the env var, but i can live with it. just
changing the format2 to yamlformat, format1 to pythonformat and it lgtm.

https://codereview.appspot.com/6308044/diff/11004/juju/charm/config.py
File juju/charm/config.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/charm/config.py#newcode8
juju/charm/config.py:8: from juju.lib.schema import (SchemaError,
KeyDict, Dict, String,
please call this yamlformat, format2 is not what the output format is.

https://codereview.appspot.com/6308044/diff/11004/juju/charm/metadata.py
File juju/charm/metadata.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/charm/metadata.py#newcode143
juju/charm/metadata.py:143: """Optional charm format, defaults to 1"""
minor, a pointer to a reference on what these entail would be useful
here.

https://codereview.appspot.com/6308044/diff/11004/juju/hooks/protocol.py
File juju/hooks/protocol.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/hooks/protocol.py#newcode405
juju/hooks/protocol.py:405: formatter = get_charm_formatter_from_env()
you don't need this here, this is an internal transport detail, you can
just pick a format, and that removes the need for env formatter
selection. the server knowing the charm format can specify it in its
response.

https://codereview.appspot.com/6308044/

[Jim Baker (jimbaker)]

On 2012/06/19 20:01:09, hazmat wrote:
> i'd still like to get rid of the env var, but i can live with it. just
changing
> the format2 to yamlformat, format1 to pythonformat and it lgtm.

Please note that format: 1 implies both Python str encoding and the use
of JSON dump/load which brings in Unicode promotion of strings (part of
the underlying bugginess). So that's why I chose to use Format1 here.
format:2 cleans this up by using YAML throughout, matching the
serialization into ZK for relation settings.

However, it's certainly easy enough to change to PythonFormat and
YAMLFormat, just wanted to explain the rationale here.

  https://codereview.appspot.com/6308044/diff/11004/juju/charm/config.py
> File juju/charm/config.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/charm/config.py#newcode8
> juju/charm/config.py:8: from juju.lib.schema import (SchemaError,
KeyDict, Dict,
> String,
> please call this yamlformat, format2 is not what the output format is.

https://codereview.appspot.com/6308044/diff/11004/juju/charm/metadata.py
> File juju/charm/metadata.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/charm/metadata.py#newcode143
> juju/charm/metadata.py:143: """Optional charm format, defaults to 1"""
> minor, a pointer to a reference on what these entail would be useful
here.

https://codereview.appspot.com/6308044/diff/11004/juju/hooks/protocol.py
> File juju/hooks/protocol.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/hooks/protocol.py#newcode405
> juju/hooks/protocol.py:405: formatter = get_charm_formatter_from_env()
> you don't need this here, this is an internal transport detail, you
can just
> pick a format, and that removes the need for env formatter selection.
the server
> knowing the charm format can specify it in its response.

https://codereview.appspot.com/6308044/

[Jim Baker (jimbaker)]

Responded to Kapil's comments.

https://codereview.appspot.com/6308044/diff/11004/juju/charm/config.py
File juju/charm/config.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/charm/config.py#newcode8
juju/charm/config.py:8: from juju.lib.schema import (SchemaError,
KeyDict, Dict, String,
On 2012/06/19 20:01:09, hazmat wrote:
> please call this yamlformat, format2 is not what the output format is.

Ack.

https://codereview.appspot.com/6308044/diff/11004/juju/charm/metadata.py
File juju/charm/metadata.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/charm/metadata.py#newcode143
juju/charm/metadata.py:143: """Optional charm format, defaults to 1"""
On 2012/06/19 20:01:09, hazmat wrote:
> minor, a pointer to a reference on what these entail would be useful
here.

Ack.

https://codereview.appspot.com/6308044/diff/11004/juju/hooks/protocol.py
File juju/hooks/protocol.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/hooks/protocol.py#newcode405
juju/hooks/protocol.py:405: formatter = get_charm_formatter_from_env()
On 2012/06/19 20:01:09, hazmat wrote:
> you don't need this here, this is an internal transport detail, you
can just
> pick a format, and that removes the need for env formatter selection.
the server
> knowing the charm format can specify it in its response.

Actually I tried this early on, and found that the proposed strategy
doesn't work. The problem is that format: 1 is sensitive to the fact
that it uses a JSON encoding/decoding step, and this is tested, among
other places, by juju.hooks.tests.test_invoker.TestCharmFormatV1
(changed to TestPythonFormat). William raised the same issue in his
report, and we discussed on IRC earlier. See
https://codereview.appspot.com/6308044/diff/1/juju/hooks/protocol.py#newcode219
and https://codereview.appspot.com/6308044/#msg7. In particular, this
would make the unicode promotion from str go away. That's a nice thing,
but bugginess is what we are trying to preserve.

https://codereview.appspot.com/6308044/diff/11004/juju/hooks/tests/test_invoker.py
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/hooks/tests/test_invoker.py#newcode511
juju/hooks/tests/test_invoker.py:511: @defer.inlineCallbacks
On 2012/06/18 22:46:41, fwereade wrote:
> Heh, nice catches :).
Thanks!

https://codereview.appspot.com/6308044/diff/11004/juju/lib/format.py
File juju/lib/format.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/lib/format.py#newcode144
juju/lib/format.py:144: os.environ.get("_JUJU_CHARM_FORMAT", "1")))
On 2012/06/18 22:46:41, fwereade wrote:
> Thanks, all this is much clearer.

Cool!

https://codereview.appspot.com/6308044/

=== modified file 'juju/charm/config.py'
--- juju/charm/config.py	2012-03-07 21:19:46 +0000
+++ juju/charm/config.py	2012-06-15 02:00:25 +0000
@@ -4,6 +4,7 @@
 
 import yaml
 
+from juju.lib.format import Format2
 from juju.lib.schema import (SchemaError, KeyDict, Dict, String,
                                  Constant, OneOf, Int, Float)
 from juju.charm.errors import (
@@ -138,8 +139,12 @@
         value, valid = validator(value, self._data[name])
 
         if not valid:
+            # Return value such that it roundtrips; this allows us to
+            # report back the boolean false instead of the Python
+            # output format, False
             raise ServiceConfigValueError(
-                "Invalid value for %s: %r" % (name, value))
+                "Invalid value for %s: %s" % (
+                    name, Format2().format(value)))
         return value
 
     def get_defaults(self):

=== modified file 'juju/charm/metadata.py'
--- juju/charm/metadata.py	2012-02-28 01:47:53 +0000
+++ juju/charm/metadata.py	2012-06-15 02:00:25 +0000
@@ -5,20 +5,17 @@
 
 from juju.charm.errors import MetaDataError
 from juju.errors import FileNotFound
+from juju.lib.format import is_valid_charm_format
 from juju.lib.schema import (
     SchemaError, Bool, Constant, Dict, Int,
     KeyDict, OneOf, UnicodeOrString)
 
 
 log = logging.getLogger("juju.charm")
-
-
 UTF8_SCHEMA = UnicodeOrString("utf-8")
-
 SCOPE_GLOBAL = "global"
 SCOPE_CONTAINER = "container"
 
-
 INTERFACE_SCHEMA = KeyDict({
     "interface": UTF8_SCHEMA,
     "limit": OneOf(Constant(None), Int()),
@@ -92,11 +89,14 @@
     "revision": Int(),
     "summary": UTF8_SCHEMA,
     "description": UTF8_SCHEMA,
+    "format": Int(),
     "peers": Dict(UTF8_SCHEMA, InterfaceExpander(limit=1)),
     "provides": Dict(UTF8_SCHEMA, InterfaceExpander(limit=None)),
     "requires": Dict(UTF8_SCHEMA, InterfaceExpander(limit=1)),
     "subordinate": Bool(),
-    }, optional=set(["provides", "requires", "peers", "revision", "subordinate"]))
+    }, optional=set(
+        ["format", "provides", "requires", "peers", "revision",
+         "subordinate"]))
 
 
 class MetaData(object):
@@ -139,6 +139,11 @@
         return self._data.get("description")
 
     @property
+    def format(self):
+        """Optional charm format, defaults to 1"""
+        return self._data.get("format", 1)
+
+    @property
     def provides(self):
         """The charm provides relations."""
         return self._data.get("provides")
@@ -211,7 +216,8 @@
             for rel in self.provides:
                 if rel.startswith("juju-"):
                     raise MetaDataError(
-                        "Charm %s attempting to provide relation in implicit relation namespace: %s" %
+                        "Charm %s attempting to provide relation in "
+                        "implicit relation namespace: %s" %
                         (self.name, rel))
                 interface = self.provides[rel]["interface"]
                 if interface.startswith("juju-"):
@@ -230,6 +236,10 @@
                     "%s labeled subordinate but lacking scope:container `requires` relation" %
                     path)
 
+        if not is_valid_charm_format(self.format):
+            raise MetaDataError("Charm %s uses an unknown format: %s" % (
+                    self.name, self.format))
+
     def parse_serialization_data(self, serialization_data, path=None):
         """Parse the unprocessed serialization data and load in this instance.
 

=== added directory 'juju/charm/tests/repository/series/mysql-format-v2'
=== added file 'juju/charm/tests/repository/series/mysql-format-v2/config.yaml'
--- juju/charm/tests/repository/series/mysql-format-v2/config.yaml	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/mysql-format-v2/config.yaml	2012-06-15 02:00:25 +0000
@@ -0,0 +1,17 @@
+options:
+    query-cache-size:
+        default: -1
+        type: int
+        description: Override the computed version from dataset-size. Still works if query-cache-type is "OFF" since sessions can override the cache type setting on their own.
+    awesome:
+        default: false
+        type: boolean
+        description: Set true to make this database engine truly awesome
+    tuning-level:
+        default: safest
+        type: string
+        description: Valid values are 'safest', 'fast', and 'unsafe'. If set to safest, all settings are tuned to have maximum safety at the cost of performance. Fast will turn off most controls, but may lose data on crashes. unsafe will turn off all protections.
+    monkey-madness:
+        default: 0.5
+        type: float
+        description: The amount of randomness to be desired in any data that is returned, from 0 (sane) to 1 (monkeys running the asylum).

=== added file 'juju/charm/tests/repository/series/mysql-format-v2/metadata.yaml'
--- juju/charm/tests/repository/series/mysql-format-v2/metadata.yaml	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/mysql-format-v2/metadata.yaml	2012-06-15 02:00:25 +0000
@@ -0,0 +1,6 @@
+name: mysql-format-v2
+summary: "Database engine"
+description: "A pretty popular database"
+provides:
+  server: mysql
+format: 2

=== added file 'juju/charm/tests/repository/series/mysql-format-v2/revision'
--- juju/charm/tests/repository/series/mysql-format-v2/revision	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/mysql-format-v2/revision	2012-06-15 02:00:25 +0000
@@ -0,0 +1,1 @@
+1
\ No newline at end of file

=== added file 'juju/charm/tests/repository/series/mysql/config.yaml'
--- juju/charm/tests/repository/series/mysql/config.yaml	1970-01-01 00:00:00 +0000
+++ juju/charm/tests/repository/series/mysql/config.yaml	2012-06-15 02:00:25 +0000
@@ -0,0 +1,17 @@
+options:
+    query-cache-size:
+        default: -1
+        type: int
+        description: Override the computed version from dataset-size. Still works if query-cache-type is "OFF" since sessions can override the cache type setting on their own.
+    awesome:
+        default: false
+        type: boolean
+        description: Set true to make this database engine truly awesome
+    tuning-level:
+        default: safest
+        type: string
+        description: Valid values are 'safest', 'fast', and 'unsafe'. If set to safest, all settings are tuned to have maximum safety at the cost of performance. Fast will turn off most controls, but may lose data on crashes. unsafe will turn off all protections.
+    monkey-madness:
+        default: 0.5
+        type: float
+        description: The amount of randomness to be desired in any data that is returned, from 0 (sane) to 1 (monkeys running the asylum).

=== modified file 'juju/charm/tests/repository/series/mysql/metadata.yaml'
--- juju/charm/tests/repository/series/mysql/metadata.yaml	2011-10-04 19:13:12 +0000
+++ juju/charm/tests/repository/series/mysql/metadata.yaml	2012-06-15 02:00:25 +0000
@@ -3,3 +3,4 @@
 description: "A pretty popular database"
 provides:
   server: mysql
+format: 1

=== modified file 'juju/charm/tests/test_config.py'
--- juju/charm/tests/test_config.py	2012-03-07 21:19:46 +0000
+++ juju/charm/tests/test_config.py	2012-06-15 02:00:25 +0000
@@ -89,7 +89,7 @@
                         "type": "string",
                         "default": True}}}))
         self.assertEqual(
-            str(e), "Invalid value for foobar: True")
+            str(e), "Invalid value for foobar: true")
 
     def test_as_dict(self):
         # load valid data
@@ -160,7 +160,7 @@
 
         error = self.assertRaises(ServiceConfigValueError,
                                   self.config.validate, {"title": True})
-        self.assertEquals(str(error), "Invalid value for title: True")
+        self.assertEquals(str(error), "Invalid value for title: true")
 
         data = self.config.validate({"title": u"Good"})
         self.assertEqual(data, {"title": u"Good"})
@@ -184,7 +184,7 @@
 
         error = self.assertRaises(ServiceConfigValueError,
                                   self.config.validate, {"skill-level": "NaN"})
-        self.assertEquals(str(error), "Invalid value for skill-level: 'NaN'")
+        self.assertEquals(str(error), "Invalid value for skill-level: NaN")
 
         data = self.config.validate({"skill-level": "9001"})
         # its over 9000!

=== modified file 'juju/charm/tests/test_metadata.py'
--- juju/charm/tests/test_metadata.py	2012-02-28 01:47:53 +0000
+++ juju/charm/tests/test_metadata.py	2012-06-15 02:00:25 +0000
@@ -60,6 +60,7 @@
         self.assertEquals(self.metadata.summary, None)
         self.assertEquals(self.metadata.description, None)
         self.assertEquals(self.metadata.is_subordinate, False)
+        self.assertEquals(self.metadata.format, 1)
 
     def test_parse_and_check_basic_info(self):
         """
@@ -238,6 +239,29 @@
             "Charm dummy attempting to provide interface in implicit namespace: juju-magic (relation: foo-rel)",
             str(error))
 
+    def test_format(self):
+        # Defaults to 1
+        self.metadata.parse(self.sample)
+        self.assertEquals(self.metadata.format, 1)
+
+        # Explicitly set to 1
+        with self.change_sample() as data:
+            data["format"] = 1
+        self.metadata.parse(self.sample)
+        self.assertEquals(self.metadata.format, 1)
+
+        # Explicitly set to 2
+        with self.change_sample() as data:
+            data["format"] = 2
+        self.metadata.parse(self.sample)
+        self.assertEquals(self.metadata.format, 2)
+
+        # Explicitly set to 3; however this is an unknown format for Juju
+        with self.change_sample() as data:
+            data["format"] = 3
+        error = self.assertRaises(MetaDataError, self.metadata.parse, self.sample)
+        self.assertIn("Charm dummy uses an unknown format: 3", str(error))
+
 
 class ParseTest(TestCase):
     """Test the parsing of some well-known sample files"""

=== modified file 'juju/control/config_get.py'
--- juju/control/config_get.py	2012-01-30 22:20:37 +0000
+++ juju/control/config_get.py	2012-06-15 02:00:25 +0000
@@ -1,9 +1,9 @@
 import argparse
-import yaml
 
 from twisted.internet.defer import inlineCallbacks
 
 from juju.control.utils import get_environment
+from juju.lib.format import Format2
 from juju.state.service import ServiceStateManager
 
 
@@ -59,38 +59,37 @@
     """
     provider = environment.get_machine_provider()
     client = yield provider.connect()
-
-    # Get the service
-    service_manager = ServiceStateManager(client)
-    service = yield service_manager.get_service_state(service_name)
-
-    # Retrieve schema
-    charm = yield service.get_charm_state()
-    schema = yield charm.get_config()
-    schema_dict = schema.as_dict()
-    display_dict = {"service": service.service_name,
-                    "charm": (yield service.get_charm_id()),
-                    "settings": schema_dict}
-
-    # Get current settings
-    settings = yield service.get_config()
-    settings = dict(settings.items())
-
-    # Merge current settings into schema/display dict
-    for k, v in schema_dict.items():
-        # Display defaults for unset values.
-        if k in settings:
-            v['value'] = settings[k]
-        else:
-            v['value'] = "-Not set-"
-
-        if 'default' in v:
-            if v['default'] == settings[k]:
-                v['default'] = True
+    try:
+        # Get the service
+        service_manager = ServiceStateManager(client)
+        service = yield service_manager.get_service_state(service_name)
+
+        # Retrieve schema
+        charm = yield service.get_charm_state()
+        schema = yield charm.get_config()
+        schema_dict = schema.as_dict()
+        display_dict = {"service": service.service_name,
+                        "charm": (yield service.get_charm_id()),
+                        "settings": schema_dict}
+
+        # Get current settings
+        settings = yield service.get_config()
+        settings = dict(settings.items())
+
+        # Merge current settings into schema/display dict
+        for k, v in schema_dict.items():
+            # Display defaults for unset values.
+            if k in settings:
+                v['value'] = settings[k]
             else:
-                del v['default']
-
-    print yaml.safe_dump(
-        display_dict,
-        indent=4, default_flow_style=False, width=80, allow_unicode=True)
-    client.close()
+                v['value'] = "-Not set-"
+
+            if 'default' in v:
+                if v['default'] == settings[k]:
+                    v['default'] = True
+                else:
+                    del v['default']
+
+        print Format2().format(display_dict)
+    finally:
+        yield client.close()

=== modified file 'juju/control/config_set.py'
--- juju/control/config_set.py	2012-04-04 13:00:13 +0000
+++ juju/control/config_set.py	2012-06-15 02:00:25 +0000
@@ -6,7 +6,7 @@
 
 from juju.charm.errors import ServiceConfigValueError
 from juju.control.utils import get_environment
-from juju.hooks.cli import parse_keyvalue_pairs
+from juju.lib.format import get_charm_formatter
 from juju.state.service import ServiceStateManager
 
 
@@ -62,7 +62,7 @@
 
         yaml_data = options.config.read()
         try:
-            data = yaml.load(yaml_data)
+            data = yaml.safe_load(yaml_data)
         except yaml.YAMLError:
             raise ServiceConfigValueError(
                 "Config file %r not valid YAML" % options.config.name)
@@ -93,6 +93,8 @@
     service_manager = ServiceStateManager(client)
     service = yield service_manager.get_service_state(service_name)
     charm = yield service.get_charm_state()
+    charm_format = (yield charm.get_metadata()).format
+    formatter = get_charm_formatter(charm_format)
 
     # Use the charm's ConfigOptions instance to validate the
     # arguments to config_set. Invalid options passed to this method
@@ -100,7 +102,7 @@
     if isinstance(service_options, dict):
         options = service_options
     else:
-        options = parse_keyvalue_pairs(service_options)
+        options = formatter.parse_keyvalue_pairs(service_options)
 
     config = yield charm.get_config()
     # ignore the output of validate, we run it so it might throw an exception

=== modified file 'juju/control/tests/test_config_set.py'
--- juju/control/tests/test_config_set.py	2012-03-27 22:46:44 +0000
+++ juju/control/tests/test_config_set.py	2012-06-15 02:00:25 +0000
@@ -182,3 +182,137 @@
         yield state.read()
         self.assertEqual(state, {"foo": "new foo",
                                  "bar": "new bar"})
+
+    @inlineCallbacks
+    def test_boolean_option_invalid_format_v1(self):
+        """Verify not possible to set a boolean option with format v1"""
+        self.service_state = yield self.add_service_from_charm("mysql")
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+        main(["set", "mysql","awesome=true"])
+        yield finished
+        self.assertEqual(
+            self.stderr.getvalue(),
+            "Invalid value for awesome: 'true'\n")
+        state = yield self.service_state.get_config()
+        self.assertEqual(
+            state,
+            {"awesome": False, "monkey-madness": 0.5,
+             "query-cache-size": -1, "tuning-level": "safest"})
+
+    @inlineCallbacks
+    def test_int_option_coerced_format_v1(self):
+        """Verify int feasible, but coerced internally to str in format v1"""
+        self.service_state = yield self.add_service_from_charm("mysql")
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+        main(["set", "mysql","query-cache-size=10"])
+        yield finished
+        self.assertEqual(self.stderr.getvalue(), "")
+        state = yield self.service_state.get_config()
+        self.assertEqual(
+            state,
+            {"awesome": False, "monkey-madness": 0.5,
+             "query-cache-size": "10", "tuning-level": "safest"})
+
+    @inlineCallbacks
+    def test_float_option_invalid_format_v1(self):
+        """Verify not possible to set a float option with format v1"""
+        self.service_state = yield self.add_service_from_charm("mysql")
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+        main(["set", "mysql","monkey-madness=0.99999999"])
+        yield finished
+        self.assertEqual(
+            self.stderr.getvalue(),
+            "Invalid value for monkey-madness: '0.99999999'\n")
+        state = yield self.service_state.get_config()
+        self.assertEqual(
+            state,
+            {"awesome": False, "monkey-madness": 0.5,
+             "query-cache-size": -1, "tuning-level": "safest"})
+
+    @inlineCallbacks
+    def test_valid_options_format_v2(self):
+        """Verify that config settings can be properly parsed and applied"""
+        self.service_state = yield self.add_service_from_charm(
+            "mysql-format-v2")
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+        main(["set",
+              "mysql-format-v2",
+              "query-cache-size=100",
+              "awesome=true",
+              "tuning-level=unsafe",
+              "monkey-madness=0.97"])
+        yield finished
+        self.assertEqual(self.stderr.getvalue(), "")
+        state = yield self.service_state.get_config()
+        self.assertEqual(
+            state,
+            {"awesome": True, "monkey-madness": 0.97,
+             "query-cache-size": 100, "tuning-level": "unsafe"})
+
+    @inlineCallbacks
+    def test_invalid_float_option_format_v2(self):
+        """Verify that config settings reject invalid floats"""
+        self.service_state = yield self.add_service_from_charm(
+            "mysql-format-v2")
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+        main(["set", "mysql-format-v2",
+              "monkey-madness='barrels of monkeys'"])
+        yield finished
+        self.assertEqual(
+            self.stderr.getvalue(), 
+            "Invalid value for monkey-madness: barrels of monkeys\n")
+        state = yield self.service_state.get_config()
+        self.assertEqual(
+            state,
+            {"awesome": False, "monkey-madness": 0.5,
+             "query-cache-size": -1, "tuning-level": "safest"})
+
+    @inlineCallbacks
+    def test_invalid_int_option_format_v2(self):
+        """Verify that config settings reject invalid ints"""
+        self.service_state = yield self.add_service_from_charm(
+            "mysql-format-v2")
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+        main(["set", "mysql-format-v2", "query-cache-size=big"])
+        yield finished
+        self.assertEqual(
+            self.stderr.getvalue(), 
+            "Invalid value for query-cache-size: big\n")
+        state = yield self.service_state.get_config()
+        self.assertEqual(
+            state,
+            {"awesome": False, "monkey-madness": 0.5,
+             "query-cache-size": -1, "tuning-level": "safest"})
+
+    @inlineCallbacks
+    def test_invalid_string_option_format_v2(self):
+        """Verify that config settings reject invalid string"""
+        self.service_state = yield self.add_service_from_charm(
+            "mysql-format-v2")
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+        # YAML does a fair amount of coercion... note that it needs to
+        # be quoted if it looks like a boolean/int/string
+        main(["set", "mysql-format-v2", "tuning-level=FALSE"])
+        yield finished
+        self.assertEqual(
+            self.stderr.getvalue(), 
+            "Invalid value for tuning-level: false\n")
+        state = yield self.service_state.get_config()
+        self.assertEqual(
+            state,
+            {"awesome": False, "monkey-madness": 0.5,
+             "query-cache-size": -1, "tuning-level": "safest"})

=== modified file 'juju/hooks/cli.py'
--- juju/hooks/cli.py	2012-05-02 00:49:10 +0000
+++ juju/hooks/cli.py	2012-06-15 02:00:25 +0000
@@ -8,8 +8,9 @@
 from twisted.internet import defer
 from twisted.internet import protocol
 
-from juju.errors import JujuError
 from juju.hooks.protocol import UnitAgentClient
+from juju.lib.format import get_charm_formatter_from_env
+
 
 _marker = object()
 
@@ -37,7 +38,6 @@
 
     `require_cid` -- Does the command require the specification of a
       client_id. (default: True)
-
     """
 
     default_mode = "wb"
@@ -153,7 +153,8 @@
             stream=self.options.log_file)
 
     def parse_kvpairs(self, options):
-        data = parse_keyvalue_pairs(options)
+        formatter = get_charm_formatter_from_env()
+        data = formatter.parse_keyvalue_pairs(options)
         # cache
         self.options.keyvalue_pairs = data
         return data
@@ -226,7 +227,8 @@
 
     def format_smart(self, result, stream):
         if result is not None:
-            print >>stream, str(result)
+            charm_formatter = get_charm_formatter_from_env()
+            print >>stream, charm_formatter.format(result)
 
 
 def parse_log_level(level):
@@ -244,31 +246,6 @@
     return level
 
 
-def parse_keyvalue_pairs(options):
-    data = {}
-    for kv in options:
-        if "=" not in kv:
-            raise JujuError(
-                "Expected `option=value`. Found `%s`" % kv)
-
-        k, v = kv.split("=", 1)
-        if v.startswith("@"):
-            # handle fileinput per spec
-            # XXX: todo -- sanitize input
-            filename = v[1:]
-            try:
-                with open(filename, "r") as f:
-                    v = f.read()
-            except IOError:
-                raise JujuError(
-                    "No such file or directory: %s (argument:%s)" % (
-                        filename,
-                        k))
-        data[k] = v
-
-    return data
-
-
 def parse_port_protocol(port_protocol_string):
     """Returns (`port`, `protocol`) by converting `port_protocol_string`.
 

=== modified file 'juju/hooks/invoker.py'
--- juju/hooks/invoker.py	2012-05-01 00:26:25 +0000
+++ juju/hooks/invoker.py	2012-06-15 02:00:25 +0000
@@ -148,6 +148,7 @@
         self._socket_path = socket_path
         self._unit_path = unit_path
         self._log = logger
+        self._charm_format = None
 
         # The twisted.internet.process.Process instance.
         self._process = None
@@ -187,6 +188,14 @@
                 display_parent_relation_ident,
                 sorted(relation_idents)))
 
+        service = yield self._context.get_local_service()
+        charm = yield service.get_charm_state()
+        self._charm_format = (yield charm.get_metadata()).format
+
+    @property
+    def charm_format(self):
+        return self._charm_format
+
     @property
     def ended(self):
         return self._ended
@@ -205,6 +214,7 @@
         base = dict(JUJU_AGENT_SOCKET=self._socket_path,
                     JUJU_CLIENT_ID=self._client_id,
                     CHARM_DIR=os.path.join(self._unit_path, "charm"),
+                    _JUJU_CHARM_FORMAT=str(self.charm_format),
                     JUJU_UNIT_NAME=os.environ["JUJU_UNIT_NAME"],
                     DEBIAN_FRONTEND="noninteractive",
                     APT_LISTCHANGES_FRONTEND="none",

=== modified file 'juju/hooks/protocol.py'
--- juju/hooks/protocol.py	2012-04-06 20:29:41 +0000
+++ juju/hooks/protocol.py	2012-06-15 02:00:25 +0000
@@ -42,14 +42,16 @@
 
 
 """
-import json
+
 import logging
 
 from twisted.internet import defer
 from twisted.internet import protocol
 from twisted.protocols import amp
+import yaml
 
 from juju.errors import JujuError
+from juju.lib.format import get_charm_formatter, get_charm_formatter_from_env
 from juju.state.errors import UnitRelationStateNotFound
 from juju.state.hook import RelationHookContext
 
@@ -96,14 +98,15 @@
                  ("relation_id", amp.String()),
                  ("unit_name", amp.String()),
                  ("setting_name", amp.String())]
-    response = [("data", amp.String())]
+    response = [("data", amp.String()),
+                ("charm_format", amp.Integer())]
 
 
 class RelationSetCommand(BaseCommand):
     commandName = "relation_set"
     arguments = [("client_id", amp.String()),
                  ("relation_id", amp.String()),
-                 ("json_blob", amp.String())]
+                 ("blob", amp.String())]
     response = []
 
 
@@ -184,7 +187,8 @@
 
     @RelationGetCommand.responder
     @defer.inlineCallbacks
-    def relation_get(self, client_id, relation_id, unit_name, setting_name):
+    def relation_get(self,
+                     client_id, relation_id, unit_name, setting_name):
         """Get settings from a state.hook.RelationHookContext
 
         :param settings_name: optional setting_name (str) indicating that
@@ -192,11 +196,11 @@
 
         """
         context = self.factory.get_context(client_id)
+        invoker = self.factory.get_invoker(client_id)
         if relation_id:
             yield self.factory.log(
                 logging.DEBUG, "Getting relation %s" % relation_id)
-            context = yield self.factory.get_invoker(client_id).\
-                get_relation_hook_context(relation_id)
+            context = yield invoker.get_relation_hook_context(relation_id)
         require_relation_context(context)
 
         try:
@@ -207,26 +211,32 @@
         except UnitRelationStateNotFound, e:
             raise NoSuchUnit(str(e))
 
-        defer.returnValue(dict(data=json.dumps(data)))
+        formatter = get_charm_formatter(invoker.charm_format)
+        defer.returnValue(dict(
+                charm_format=invoker.charm_format,
+                data=formatter.dump(data)))
 
     @RelationSetCommand.responder
     @defer.inlineCallbacks
-    def relation_set(self, client_id, relation_id, json_blob):
+    def relation_set(self, client_id, relation_id, blob):
         """Set values into state.hook.RelationHookContext.
 
-        :param json_blob: a JSON serialized string of a dict that will
+        :param blob: a YAML or JSON dumped string of a dict that will
         contain the delta of settings to be applied to a unit_name.
         """
-        data = json.loads(json_blob)
         context = yield self.factory.get_context(client_id)
+        invoker = self.factory.get_invoker(client_id)
+        formatter = get_charm_formatter(invoker.charm_format)
+        data = formatter.load(blob)
+
         if relation_id:
             yield self.factory.log(
                 logging.DEBUG, "Setting relation %s" % relation_id)
-            context = yield self.factory.get_invoker(client_id).\
-                get_relation_hook_context(relation_id)
+            context = yield invoker.get_relation_hook_context(relation_id)
         require_relation_context(context)
+
         for k, v in data.items():
-            if not v.strip():
+            if formatter.should_delete(v):
                 yield context.delete_value(k)
             else:
                 yield context.set_value(k, v)
@@ -298,7 +308,9 @@
         else:
             options = dict(options)
 
-        defer.returnValue(dict(data=json.dumps(options)))
+        # NOTE: no need to consider charm format for blob here, this
+        # blob has always been in YAML format
+        defer.returnValue(dict(data=yaml.safe_dump(options)))
 
     @OpenPortCommand.responder
     @defer.inlineCallbacks
@@ -380,7 +392,8 @@
                                        relation_id=relation_id,
                                        unit_name=unit_name,
                                        setting_name=setting_name)
-        defer.returnValue(json.loads(result["data"]))
+        formatter = get_charm_formatter(result["charm_format"])
+        defer.returnValue(formatter.load(result["data"]))
 
     @defer.inlineCallbacks
     def relation_set(self, client_id, relation_id, data):
@@ -389,11 +402,12 @@
         :param data: Python dict applied as a delta hook settings
 
         """
-        json_blob = json.dumps(data)
+        formatter = get_charm_formatter_from_env()
+        blob = formatter.dump(data)
         yield self.callRemote(RelationSetCommand,
                               client_id=client_id,
                               relation_id=relation_id,
-                              json_blob=json_blob)
+                              blob=blob)
         defer.returnValue(None)
 
     @defer.inlineCallbacks
@@ -429,7 +443,7 @@
                                        client_id=client_id,
                                        option_name=option_name)
         # Unbundle and deserialize
-        result = json.loads(result["data"])
+        result = yaml.safe_load(result["data"])
         defer.returnValue(result)
 
     @defer.inlineCallbacks

=== modified file 'juju/hooks/tests/test_cli.py'
--- juju/hooks/tests/test_cli.py	2012-03-27 01:04:50 +0000
+++ juju/hooks/tests/test_cli.py	2012-06-15 02:00:25 +0000
@@ -1,15 +1,17 @@
+# -*- encoding: utf-8 -*-
+
 import json
 import logging
 import os
 import StringIO
 from argparse import ArgumentTypeError
+from contextlib import closing
 
 from twisted.internet.defer import inlineCallbacks, returnValue
+import yaml
 
-from juju.errors import JujuError
 from juju.hooks.cli import (
-    CommandLineClient, parse_keyvalue_pairs, parse_log_level,
-    parse_port_protocol)
+    CommandLineClient, parse_log_level, parse_port_protocol)
 from juju.lib.testing import TestCase
 
 
@@ -322,30 +324,6 @@
         self.assertEquals(parse_log_level(logging.INFO), logging.INFO)
         self.assertEquals(parse_log_level(logging.ERROR), logging.ERROR)
 
-    def test_parse_keyvalue_pairs(self):
-        sample = self.makeFile("INPUT DATA")
-
-        # test various styles of options being read
-        options = ["alpha=beta",
-                   "content=@%s" % sample]
-
-        data = parse_keyvalue_pairs(options)
-        self.assertEquals(data["alpha"], "beta")
-        self.assertEquals(data["content"], "INPUT DATA")
-
-        # and check an error condition
-        options = ["content=@missing"]
-        error = self.assertRaises(JujuError, parse_keyvalue_pairs, options)
-        self.assertEquals(
-            str(error),
-            "No such file or directory: missing (argument:content)")
-
-        # and check when fed non-kvpairs the error makes sense
-        options = ["foobar"]
-        error = self.assertRaises(JujuError, parse_keyvalue_pairs, options)
-        self.assertEquals(
-            str(error), "Expected `option=value`. Found `foobar`")
-
     def test_parse_port_protocol(self):
         self.assertEqual(parse_port_protocol("80"), (80, "tcp"))
         self.assertEqual(parse_port_protocol("443/tcp"), (443, "tcp"))
@@ -388,33 +366,96 @@
             "Invalid protocol, must be 'tcp' or 'udp', "
             "got 'not-a-valid-protocol'")
 
-    def test_format_smart(self):
-        cli = CommandLineClient()
-
-        output = StringIO.StringIO()
-        cli.format_smart(str('Basic string'), output)
-        self.assertEqual(output.getvalue(), "Basic string\n")
-        output.close()
-
-        output = StringIO.StringIO()
-        cli.format_smart(float(1.0), output)
-        self.assertEqual(output.getvalue(), "1.0\n")
-        output.close()
-
-        output = StringIO.StringIO()
-        cli.format_smart(int(1), output)
-        self.assertEqual(output.getvalue(), "1\n")
-        output.close()
-
-        # LP bug # 900517 - smart format renders int(0) as an empty
-        # string
-        output = StringIO.StringIO()
-        cli.format_smart(int(0), output)
-        self.assertEqual(output.getvalue(), "0\n")
-        output.close()
-
-        # None does not even print the \n
-        output = StringIO.StringIO()
-        cli.format_smart(None, output)
-        self.assertEqual(output.getvalue(), "")
-        output.close()
+    def assert_smart_output_v1(self, sample, formatted=object()):
+        """Verifies output serialization"""
+        # No roundtripping is verified because str(obj) is in general
+        # not roundtrippable
+        cli = CommandLineClient()
+        with closing(StringIO.StringIO()) as output:
+            cli.format_smart(sample, output)
+            self.assertEqual(output.getvalue(), formatted)
+
+    def assert_format_smart_v1(self):
+        """Verifies legacy smart format v1 which uses Python str encoding"""
+        self.assert_smart_output_v1(None, "")  # No \n in output for None
+        self.assert_smart_output_v1("", "\n")
+        self.assert_smart_output_v1("A string", "A string\n")
+        self.assert_smart_output_v1(
+            "High bytes: \xca\xfe", "High bytes: \xca\xfe\n")
+        self.assert_smart_output_v1(u"", "\n")
+        self.assert_smart_output_v1(
+            u"A unicode string (but really ascii)",
+            "A unicode string (but really ascii)\n")
+        # Maintain LP bug #901495, fixed in v2 format; this happens because
+        # str(obj) is used
+        e = self.assertRaises(
+            UnicodeEncodeError,
+            self.assert_smart_output_v1, u"中文")
+        self.assertEqual(
+            str(e),
+            ("'ascii' codec can't encode characters in position 0-1: "
+             "ordinal not in range(128)"))
+        self.assert_smart_output_v1({}, "{}\n")
+        self.assert_smart_output_v1(
+            {u"public-address": u"ec2-1-2-3-4.compute-1.amazonaws.com"},
+            "{u'public-address': u'ec2-1-2-3-4.compute-1.amazonaws.com'}\n")
+        self.assert_smart_output_v1(False, "False\n")
+        self.assert_smart_output_v1(True, "True\n")
+        self.assert_smart_output_v1(0.0, "0.0\n")
+        self.assert_smart_output_v1(3.14159, "3.14159\n")
+        self.assert_smart_output_v1(6.02214178e23, "6.02214178e+23\n")
+        self.assert_smart_output_v1(0, "0\n")
+        self.assert_smart_output_v1(42, "42\n")
+
+    def test_format_smart_v1_implied(self):
+        """Smart format v1 is implied if _JUJU_CHARM_FORMAT is not defined"""
+        # Double check env setup
+        self.assertNotIn("_JUJU_CHARM_FORMAT", os.environ)
+        self.assert_format_smart_v1()
+
+    def test_format_smart_v1(self):
+        """Verify legacy format v1 works"""
+        self.change_environment(_JUJU_CHARM_FORMAT="1")
+        self.assert_format_smart_v1()
+
+    def assert_smart_output(self, sample, formatted):
+        cli = CommandLineClient()
+        with closing(StringIO.StringIO()) as output:
+            cli.format_smart(sample, output)
+            self.assertEqual(output.getvalue(), formatted)
+            self.assertEqual(sample, yaml.safe_load(output.getvalue()))
+
+    def test_format_smart_v2(self):
+        """Verifies smart format v2 writes correct YAML"""
+        self.change_environment(_JUJU_CHARM_FORMAT="2")
+
+        # For each case, verify actual output serialization along with
+        # roundtripping through YAML
+        self.assert_smart_output(None, "")  # No newline in output for None
+        self.assert_smart_output("", "''\n")
+        self.assert_smart_output("A string", "A string\n")
+        # Note: YAML uses b64 encoding for byte strings tagged by !!binary
+        self.assert_smart_output(
+            "High bytes: \xCA\xFE",
+            "!!binary |\n    SGlnaCBieXRlczogyv4=\n")
+        self.assert_smart_output(u"", "''\n")
+        self.assert_smart_output(
+            u"A unicode string (but really ascii)",
+            "A unicode string (but really ascii)\n")
+        # Any non-ascii Unicode will use UTF-8 encoding
+        self.assert_smart_output(u"中文", "\xe4\xb8\xad\xe6\x96\x87\n")
+        self.assert_smart_output({}, "{}\n")
+        self.assert_smart_output(
+            {u"public-address": u"ec2-1-2-3-4.compute-1.amazonaws.com",
+             u"foo": u"bar",
+             u"configured": True},
+            ("configured: true\n"
+             "foo: bar\n"
+             "public-address: ec2-1-2-3-4.compute-1.amazonaws.com\n"))
+        self.assert_smart_output(False, "false\n")
+        self.assert_smart_output(True, "true\n")
+        self.assert_smart_output(0.0, "0.0\n")
+        self.assert_smart_output(3.14159, "3.14159\n")
+        self.assert_smart_output(6.02214178e23, "6.02214178e+23\n")
+        self.assert_smart_output(0, "0\n")
+        self.assert_smart_output(42, "42\n")

=== modified file 'juju/hooks/tests/test_communications.py'
--- juju/hooks/tests/test_communications.py	2012-04-06 20:29:41 +0000
+++ juju/hooks/tests/test_communications.py	2012-06-15 02:00:25 +0000
@@ -76,6 +76,12 @@
         return MockServiceUnitState
 
 
+class MockInvoker(object):
+
+    def __init__(self, charm_format):
+        self.charm_format = charm_format
+
+
 class UnitSettingsFactoryLocal(UnitSettingsFactory):
     """
     For testing a UnitSettingsFactory with local storage. Loosely
@@ -91,6 +97,7 @@
         self.members = []
         self.relation_idents = []
         self._agent_io = StringIO()
+        self._invoker = MockInvoker(charm_format=1)
 
         # hook context and a logger to the settings factory
         logger = logging.getLogger("unit-settings-fact-test")
@@ -104,7 +111,7 @@
         return self
 
     def invoker(self, client_id):
-        return "mock invoker"
+        return self._invoker
 
     def get_value(self, unit_name, setting_name):
         return self.data[unit_name][setting_name]
@@ -310,7 +317,7 @@
         error = MustSpecifyRelationName()
         self.assertTrue(isinstance(error, JujuError))
         self.assertEquals(
-            str(error), 
+            str(error),
             "Relation name must be specified")
 
     @defer.inlineCallbacks

=== modified file 'juju/hooks/tests/test_invoker.py'
--- juju/hooks/tests/test_invoker.py	2012-05-04 03:36:36 +0000
+++ juju/hooks/tests/test_invoker.py	2012-06-15 02:00:25 +0000
@@ -1,3 +1,5 @@
+# -*- encoding: utf-8 -*-
+
 from StringIO import StringIO
 import json
 import logging
@@ -471,6 +473,7 @@
 
 class InvokerTest(RelationInvokerTestBase):
 
+    @defer.inlineCallbacks
     def test_environment(self):
         """Test various way to manipulate the calling environment.
         """
@@ -494,11 +497,18 @@
         self.assertEqual(env["DEBIAN_FRONTEND"], "noninteractive")
         self.assertEqual(env["APT_LISTCHANGES_FRONTEND"], "none")
 
+        # Specific to the charm that is running, in this case it's the
+        # dummy charm (this is the default charm used when the
+        # add_service method is use)
+        self.assertEqual(env["_JUJU_CHARM_FORMAT"], "1")
+
+    @defer.inlineCallbacks
     def test_missing_hook(self):
         exe = yield self.ua.get_invoker(
             "database:42", "add", "mysql/0", self.relation)
         self.failUnlessRaises(errors.FileNotFound, exe, "hook-missing")
 
+    @defer.inlineCallbacks
     def test_noexec_hook(self):
         exe = yield self.ua.get_invoker(
             "database:42", "add", "mysql/0", self.relation)
@@ -1080,10 +1090,10 @@
                 u"b": u"xyz",
                 "private-address": "mysql-0.example.com"})
         yield self.assert_zk_data(db1, {
-                u"c": u"21",
+                u"c": "21",
                 "private-address": "mysql-0.example.com"})
         yield self.assert_zk_data(db2, {
-                u"d": u"99",
+                u"d": "99",
                 "private-address": "mysql-0.example.com"})
         self.assertLogLines(
             self.log.getvalue(),
@@ -1502,3 +1512,166 @@
         self.assertIn("mysql/1", self.log.getvalue())
         # we don't see units in the other container
         self.assertNotIn("mysql/0", self.log.getvalue())
+
+
+def capture_separate_log(name, level):
+    """Support the separate capture of logging at different log levels.
+
+    TestCase.capture_logging only allows one level to be captured at
+    any given time. Given that the hook log captures both data AND
+    traditional logging, it's useful to separate.
+    """
+    logger = logging.getLogger(name)
+    output = StringIO()
+    handler = logging.StreamHandler(output)
+    handler.setLevel(level)
+    logger.addHandler(handler)
+    return output
+
+
+class TestCharmFormatV1(RelationInvokerTestBase):
+
+    @defer.inlineCallbacks
+    def _default_relations(self):
+        """Intercept mysql/wordpress setup to ensure v1 charm format"""
+        # The mysql charm in the test repository has no format defined
+        yield self.add_service_from_charm("mysql", charm_name="mysql")
+        yield super(TestCharmFormatV1, self)._default_relations()
+
+    @defer.inlineCallbacks
+    def test_environment(self):
+        """Ensure that an implicit setting of format: 1 works properly"""
+        exe = yield self.ua.get_invoker(
+            "database:42", "add", "mysql/0", self.relation)
+        env = exe.get_environment()
+        self.assertEqual(env["_JUJU_CHARM_FORMAT"], "1")
+
+    @defer.inlineCallbacks
+    def test_output(self):
+        """Verify roundtripping"""
+        hook_debug_log = capture_separate_log("hook", level=logging.DEBUG)
+        hook_log = capture_separate_log("hook", level=logging.INFO)
+        exe = yield self.ua.get_invoker(
+            "database:42", "add", "mysql/0", self.relation,
+            client_id="client_id")
+        set_hook = self.create_hook(
+            "relation-set",
+            "b=true i=42 f=1.23 s=ascii u=中文")
+        yield exe(set_hook)
+        result = yield exe(self.create_hook("relation-get", "- mysql/0"))
+        self.assertEqual(result, 0)
+
+        # No guarantee on output ordering, so keep to this test stable,
+        # first a little parsing work:
+        output = hook_log.getvalue()
+        self.assertEqual(output[0], "{")
+        self.assertEqual(output[-3:-1], "}\n")
+        self.assertEqual(
+            set(output[1:-3].split(", ")),
+            set(["u'b': u'true'",
+                 "u'f': u'1.23'",
+                 "u'i': u'42'",
+                 "u'private-address': u'mysql-0.example.com'",
+                 "u's': u'ascii'",
+                 "u'u': u'\\u4e2d\\u6587'"]))
+
+        self.assertLogLines(
+            hook_debug_log.getvalue(),
+            ["Flushed values for hook %r on 'database:42'" % (
+                    os.path.basename(set_hook),),
+             "    Setting changed: u'b'=u'true' (was unset)",
+             "    Setting changed: u'f'=u'1.23' (was unset)",
+             "    Setting changed: u'i'=u'42' (was unset)",
+             "    Setting changed: u's'=u'ascii' (was unset)",
+             "    Setting changed: u'u'=u'\\u4e2d\\u6587' (was unset)"])
+
+        # Unlike v2 formatting, this will only fail on output
+        hook_log.truncate()
+        data_file = self.makeFile("But when I do drink, I prefer \xCA\xFE")
+        yield exe(self.create_hook(
+                "relation-set", "d=@%s" % data_file))
+        result = yield exe(self.create_hook("relation-get", "d mysql/0"))
+        self.assertIn(
+            "Error: \'utf8\' codec can\'t decode byte 0xca in position 30: "
+            "invalid continuation byte",
+            hook_log.getvalue())
+
+
+class TestCharmFormatV2(RelationInvokerTestBase):
+
+    @defer.inlineCallbacks
+    def _default_relations(self):
+        """Intercept mysql/wordpress setup to ensure v2 charm format"""
+        # The mysql-format-v2 charm defines format:2  in its metadata
+        yield self.add_service_from_charm(
+            "mysql", charm_name="mysql-format-v2")
+        yield super(TestCharmFormatV2, self)._default_relations()
+
+    @defer.inlineCallbacks
+    def test_environment(self):
+        """Ensure that an explicit setting of format: 2 works properly"""
+        exe = yield self.ua.get_invoker(
+            "database:42", "add", "mysql/0", self.relation)
+        env = exe.get_environment()
+        self.assertEqual(env["_JUJU_CHARM_FORMAT"], "2")
+
+    @defer.inlineCallbacks
+    def test_output(self):
+        """Verify roundtripping"""
+        hook_debug_log = capture_separate_log("hook", level=logging.DEBUG)
+        hook_log = capture_separate_log("hook", level=logging.INFO)
+        exe = yield self.ua.get_invoker(
+            "database:42", "add", "mysql/0", self.relation,
+            client_id="client_id")
+
+        # Byte strings are also supported by staying completely in
+        # YAML, so test that corner case. This also means users need
+        # to present valid YAML for any input:
+        data_file = self.makeFile(
+            yaml.safe_dump("But when I do drink, I prefer \xCA\xFE"))
+        set_hook = self.create_hook(
+            "relation-set",
+            "b=true i=42 f=1.23 s=ascii u=中文 d=@%s" % data_file)
+        yield exe(set_hook)
+        result = yield exe(self.create_hook("relation-get", "- mysql/0"))
+        self.assertEqual(result, 0)
+
+        # YAML guarantees that the keys will be sorted
+        # lexicographically; note that we output UTF-8 for Unicode
+        # when dumping YAML, so our source text (with this test file
+        # in UTF-8 itself) matches the output text, as seen in the
+        # characters for "zhongwen" (Chinese language).
+        self.assertEqual(
+            hook_log.getvalue(),
+            "b: true\n"
+            "d: !!binary |\n    QnV0IHdoZW4gSSBkbyBkcmluaywgSSBwcmVmZXIgyv4=\n"
+            "f: 1.23\n"
+            "i: 42\n"
+            "private-address: mysql-0.example.com\n"
+            "s: ascii\n"
+            "u: 中文\n\n")
+
+        # Log lines are not simply converted into Unicode, as in v1 format
+        self.assertLogLines(
+            hook_debug_log.getvalue(),
+            ["Flushed values for hook %r on 'database:42'" % (
+                    os.path.basename(set_hook),),
+             "    Setting changed: 'b'=True (was unset)",
+             "    Setting changed: 'd'='But when I do drink, "
+             "I prefer \\xca\\xfe' (was unset)",
+             "    Setting changed: 'f'=1.23 (was unset)",
+             "    Setting changed: 'i'=42 (was unset)",
+             "    Setting changed: 's'='ascii' (was unset)",
+             "    Setting changed: 'u'=u'\\u4e2d\\u6587' (was unset)"])
+
+        # Also ensure that invalid YAML is rejected; unlike earlier,
+        # this was not encoded with yaml.safe_dump
+        data_file = self.makeFile(
+            "But when I do drink, I prefer \xCA\xFE")
+        hook = self.create_hook("relation-set", "d=@%s" % data_file)
+        e = yield self.assertFailure(exe(hook), errors.CharmInvocationError)
+        self.assertEqual(str(e), "Error processing %r: exit code 1." % hook)
+        self.assertIn(
+            "yaml.reader.ReaderError: \'utf8\' codec can\'t decode byte #xca: "
+            "invalid continuation byte\n  in \"<string>\", position 30",
+            hook_log.getvalue())

=== added file 'juju/lib/format.py'
--- juju/lib/format.py	1970-01-01 00:00:00 +0000
+++ juju/lib/format.py	2012-06-15 02:00:25 +0000
@@ -0,0 +1,144 @@
+"""Utility functions and constants to support uniform i/o formatting."""
+
+import json
+import os
+
+import yaml
+
+from juju.errors import JujuError
+
+
+class BaseFormat(object):
+    """Maintains parallel code paths for input and output formatting
+    through the subclasses Format1 (Python str formatting with JSON
+    encoding) and Format2 (YAML format).
+    """
+
+    def parse_keyvalue_pairs(self, pairs):
+        """Parses key value pairs, using `_parse_value` for specific format"""
+        data = {}
+        for kv in pairs:
+            if "=" not in kv:
+                raise JujuError(
+                    "Expected `option=value`. Found `%s`" % kv)
+
+            k, v = kv.split("=", 1)
+            if v.startswith("@"):
+                # Handle file input, any parsing/sanitization is done next
+                # with respect to charm format
+                filename = v[1:]
+                try:
+                    with open(filename, "r") as f:
+                        v = f.read()
+                except IOError:
+                    raise JujuError(
+                        "No such file or directory: %s (argument:%s)" % (
+                            filename,
+                            k))
+                except Exception, e:
+                    raise JujuError("Bad file %s" % e)
+
+            data[k] = self._parse_value(k, v)
+
+        return data
+
+
+class Format1(BaseFormat):
+    """Supports backwards compatibility through str and JSON encoding."""
+
+    charm_format = 1
+
+    def format(self, data):
+        """Formats `data` using Python str encoding"""
+        return str(data)
+
+    def _parse_value(self, key, value):
+        """Interprets value as a str"""
+        return value
+
+    # For the old format: 1, using JSON serialization introduces some
+    # subtle issues around Unicode conversion that then later results
+    # in bugginess. For compatibility reasons, we keep these old bugs
+    # around, by dumping and loading into JSON at appropriate points.
+
+    def dump(self, data):
+        """Dumps using JSON serialization"""
+        return json.dumps(data)
+
+    def load(self, data):
+        """Loads data, but also converts str to Unicode"""
+        return json.loads(data)
+
+    def should_delete(self, value):
+        """Whether `value` implies corresponding key should be deleted"""
+        # In format: 1, all values are strings, but possibly with
+        # spaces. The strip reduces strings consisting only of spaces,
+        # or otherwise empty, to an empty string.
+        return not value.strip()
+
+
+class Format2(BaseFormat):
+    """New format that uses YAML, so no unexpected encoding issues"""
+
+    charm_format = 2
+
+    def format(self, data):
+        """Formats `data` in Juju's preferred YAML format"""
+        # Return value such that it roundtrips; this allows us to
+        # report back the boolean false instead of the Python
+        # output format, False
+        if data is None:
+            return ""
+        serialized = yaml.safe_dump(
+            data, indent=4, default_flow_style=False, width=80,
+            allow_unicode=True)
+        if serialized.endswith("\n...\n"):
+            # Remove explicit doc end sentinel, still valid yaml
+            serialized = serialized[0:-5]
+        # Also remove any extra \n, will still be valid yaml
+        return serialized.rstrip("\n")
+
+    def _parse_value(self, key, value):
+        # Ensure roundtripping to/from yaml if format: 2; in
+        # particular this ensures that true/false work as desired
+        try:
+            return yaml.safe_load(value)
+        except Exception:
+            raise JujuError("Invalid YAML value (argument:%s)" % key)
+
+    # Use the same format for dump
+    dump = format
+
+    def load(self, data):
+        """Loads data safely, ensuring no Python specific type info leaks"""
+        return yaml.safe_load(data)
+
+    def should_delete(self, value):
+        """Whether `value` implies corresponding key should be deleted"""
+        # In format: 2, values were already parsed by yaml.safe_load;
+        # in particular, a string consisting only of whitespace is
+        # parsed as None.
+        return value is None
+
+
+def is_valid_charm_format(charm_format):
+    """True if `charm_format` is a valid format"""
+    return charm_format in (Format1.charm_format, Format2.charm_format)
+
+
+def get_charm_formatter(charm_format):
+    """Map `charm_format` to the implementing strategy for that format"""
+    if charm_format == Format1.charm_format:
+        return Format1()
+    elif charm_format == Format2.charm_format:
+        return Format2()
+    else:
+        raise JujuError(
+            "Expected charm format to be either 1 or 2, got %s" % (
+                charm_format,))
+
+
+def get_charm_formatter_from_env():
+    """Return the formatter specified by $_JUJU_CHARM_FORMAT"""
+    return get_charm_formatter(int(
+            os.environ.get("_JUJU_CHARM_FORMAT", "1")))

=== added file 'juju/lib/tests/test_format.py'
--- juju/lib/tests/test_format.py	1970-01-01 00:00:00 +0000
+++ juju/lib/tests/test_format.py	2012-06-15 02:00:25 +0000
@@ -0,0 +1,330 @@
+# -*- encoding: utf-8 -*-
+
+import json
+
+import yaml
+
+from juju.errors import JujuError
+from juju.lib.testing import TestCase
+from juju.lib.format import (
+    Format1, Format2, is_valid_charm_format, get_charm_formatter,
+    get_charm_formatter_from_env)
+
+
+class TestFormatLookup(TestCase):
+
+    def test_is_valid_charm_format(self):
+        """Verify currently valid charm formats"""
+        self.assertFalse(is_valid_charm_format(0))
+        self.assertTrue(is_valid_charm_format(1))
+        self.assertTrue(is_valid_charm_format(2))
+        self.assertFalse(is_valid_charm_format(3))
+
+    def test_get_charm_formatter(self):
+        self.assertInstance(get_charm_formatter(1), Format1)
+        self.assertInstance(get_charm_formatter(2), Format2)
+        e = self.assertRaises(JujuError, get_charm_formatter, 0)
+        self.assertEqual(
+            str(e),
+            "Expected charm format to be either 1 or 2, got 0")
+
+    def test_get_charm_formatter_from_env(self):
+        """Verifies _JUJU_CHARM_FORMAT can be mapped to valid formatters"""
+
+        self.change_environment(_JUJU_CHARM_FORMAT="0")
+        e = self.assertRaises(JujuError, get_charm_formatter_from_env)
+        self.assertEqual(
+            str(e),
+            "Expected charm format to be either 1 or 2, got 0")
+
+        self.change_environment(_JUJU_CHARM_FORMAT="1")
+        self.assertInstance(get_charm_formatter_from_env(), Format1)
+
+        self.change_environment(_JUJU_CHARM_FORMAT="2")
+        self.assertInstance(get_charm_formatter_from_env(), Format2)
+
+        self.change_environment(_JUJU_CHARM_FORMAT="3")
+        e = self.assertRaises(JujuError, get_charm_formatter_from_env)
+        self.assertEqual(
+            str(e),
+            "Expected charm format to be either 1 or 2, got 3")
+
+
+class TestFormat1(TestCase):
+
+    def assert_format(self, data, expected):
+        """Verify str output serialization; no roundtripping is supported."""
+        formatted = Format1().format(data)
+        self.assertEqual(formatted, expected)
+
+    def test_format(self):
+        """Verifies Python str formatting of data"""
+        self.assert_format(None, "None")
+        self.assert_format("", "")
+        self.assert_format("A string", "A string")
+        self.assert_format(
+            "High bytes: \xCA\xFE",
+            "High bytes: \xca\xfe")
+        self.assert_format(u"", "")
+        self.assert_format(
+            u"A unicode string (but really ascii)",
+            "A unicode string (but really ascii)")
+        e = self.assertRaises(UnicodeEncodeError, Format1().format, u"中文")
+        self.assertEqual(
+                str(e),
+                ("'ascii' codec can't encode characters in position 0-1: "
+                "ordinal not in range(128)"))
+        self.assert_format({}, "{}")
+        self.assert_format(
+            {u"public-address": u"ec2-1-2-3-4.compute-1.amazonaws.com",
+             u"foo": u"bar",
+             u"configured": True},
+            ("{u'public-address': u'ec2-1-2-3-4.compute-1.amazonaws.com', "
+             "u'foo': u'bar', u'configured': True}"))
+        self.assert_format(False, "False")
+        self.assert_format(True, "True")
+        self.assert_format(0.0, "0.0")
+        self.assert_format(3.14159, "3.14159")
+        self.assert_format(6.02214178e23, "6.02214178e+23")
+        self.assert_format(0, "0")
+        self.assert_format(42, "42")
+
+    def test_parse_keyvalue_pairs(self):
+        """Verify reads in values as strings"""
+        sample = self.makeFile("INPUT DATA")
+
+        # test various styles of options being read
+        options = ["alpha=beta",
+                   "content=@%s" % sample]
+
+        formatter = Format1()
+        data = formatter.parse_keyvalue_pairs(options)
+        self.assertEquals(data["alpha"], "beta")
+        self.assertEquals(data["content"], "INPUT DATA")
+
+        # and check an error condition
+        options = ["content=@missing"]
+        error = self.assertRaises(
+            JujuError, formatter.parse_keyvalue_pairs, options)
+        self.assertEquals(
+            str(error),
+            "No such file or directory: missing (argument:content)")
+
+        # and check when fed non-kvpairs the error makes sense
+        options = ["foobar"]
+        error = self.assertRaises(
+            JujuError, formatter.parse_keyvalue_pairs, options)
+        self.assertEquals(
+            str(error), "Expected `option=value`. Found `foobar`")
+
+    def assert_dump_load(self, data, expected):
+        """Asserts expected formatting and roundtrip through dump/load"""
+        formatter = Format1()
+        dumped = formatter.dump({"data": data})
+        loaded = formatter.load(dumped)["data"]
+        self.assertEqual(dumped, expected)
+        self.assertEqual(dumped, json.dumps({"data": data}))
+        self.assertEqual(data, loaded)
+        if isinstance(data, str):
+            # Verify promotion of str to unicode
+            self.assertInstance(loaded, unicode)
+
+    def test_dump_load(self):
+        """Verify JSON roundtrip semantics"""
+        self.assert_dump_load(None, '{"data": null}')
+        self.assert_dump_load("", '{"data": ""}')
+        self.assert_dump_load("A string", '{"data": "A string"}')
+        e = self.assertRaises(
+            UnicodeDecodeError,
+            Format1().dump, "High bytes: \xCA\xFE")
+        self.assertEqual(
+            str(e), 
+            "'utf8' codec can't decode byte 0xca in position 12: "
+            "invalid continuation byte")
+        self.assert_dump_load(u"", '{"data": ""}')
+        self.assert_dump_load(
+            u"A unicode string (but really ascii)",
+            '{"data": "A unicode string (but really ascii)"}')
+        e = self.assertRaises(UnicodeEncodeError, Format1().format, u"中文")
+        self.assertEqual(
+                str(e),
+                ("'ascii' codec can't encode characters in position 0-1: "
+                "ordinal not in range(128)"))
+        self.assert_dump_load({}, '{"data": {}}')
+        self.assert_dump_load(
+            {u"public-address": u"ec2-1-2-3-4.compute-1.amazonaws.com",
+             u"foo": u"bar",
+             u"configured": True},
+            ('{"data": {"public-address": '
+             '"ec2-1-2-3-4.compute-1.amazonaws.com", "foo": "bar", '
+             '"configured": true}}'))
+        self.assert_dump_load(False, '{"data": false}')
+        self.assert_dump_load(True, '{"data": true}')
+        self.assert_dump_load(0.0, '{"data": 0.0}')
+        self.assert_dump_load(3.14159, '{"data": 3.14159}')
+        self.assert_dump_load(6.02214178e23, '{"data": 6.02214178e+23}')
+        self.assert_dump_load(0, '{"data": 0}')
+        self.assert_dump_load(42, '{"data": 42}')
+
+    def test_should_delete(self):
+        """Verify empty or whitespace only strings indicate deletion"""
+        formatter = Format1()
+        self.assertFalse(formatter.should_delete("0"))
+        self.assertFalse(formatter.should_delete("something"))
+        self.assertTrue(formatter.should_delete(""))
+        self.assertTrue(formatter.should_delete("   "))
+
+        # Verify that format: 1 can only work with str values
+        e = self.assertRaises(AttributeError, formatter.should_delete, 42)
+        self.assertEqual(str(e), "'int' object has no attribute 'strip'")
+        e = self.assertRaises(AttributeError, formatter.should_delete, None)
+        self.assertEqual(str(e), "'NoneType' object has no attribute 'strip'")
+
+
+class TestFormat2(TestCase):
+
+    def assert_format(self, data, expected):
+        """Verify actual output serialization and roundtripping through YAML"""
+        formatted = Format2().format(data)
+        self.assertEqual(formatted, expected)
+        self.assertEqual(data, yaml.safe_load(formatted))
+
+    def test_format(self):
+        """Verifies standard formatting of data into valid YAML"""
+        self.assert_format(None, "")
+        self.assert_format("", "''")
+        self.assert_format("A string", "A string")
+        # Note: YAML uses b64 encoding for byte strings tagged by !!binary
+        self.assert_format(
+            "High bytes: \xCA\xFE",
+            "!!binary |\n    SGlnaCBieXRlczogyv4=")
+        self.assert_format(u"", "''")
+        self.assert_format(
+            u"A unicode string (but really ascii)",
+            "A unicode string (but really ascii)")
+        # Any non-ascii Unicode will use UTF-8 encoding
+        self.assert_format(u"中文", "\xe4\xb8\xad\xe6\x96\x87")
+        self.assert_format({}, "{}")
+        self.assert_format(
+            {u"public-address": u"ec2-1-2-3-4.compute-1.amazonaws.com",
+             u"foo": u"bar",
+             u"configured": True},
+            ("configured: true\n"
+             "foo: bar\n"
+             "public-address: ec2-1-2-3-4.compute-1.amazonaws.com"))
+        self.assert_format(False, "false")
+        self.assert_format(True, "true")
+        self.assert_format(0.0, "0.0")
+        self.assert_format(3.14159, "3.14159")
+        self.assert_format(6.02214178e23, "6.02214178e+23")
+        self.assert_format(0, "0")
+        self.assert_format(42, "42")
+
+    def assert_parse(self, data):
+        """Verify input parses as expected, including from a data file"""
+        formatter = Format2()
+        formatted = formatter.format(data)
+        data_file = self.makeFile(formatted)
+        kvs = ["formatted=%s" % formatted,
+               "file=@%s" % data_file]
+        parsed = formatter.parse_keyvalue_pairs(kvs)
+        self.assertEqual(parsed["formatted"], data)
+        self.assertEqual(parsed["file"], data)
+
+    def test_parse_keyvalue_pairs(self):
+        """Verify key value pairs parse for a wide range of YAML inputs."""
+        formatter = Format2()
+        self.assert_parse(None)
+        self.assert_parse("")
+        self.assert_parse("A string")
+        self.assert_parse("High bytes: \xCA\xFE")
+        self.assert_parse(u"")
+        self.assert_parse(u"A unicode string (but really ascii)")
+        self.assert_parse(u"中文")
+        self.assert_parse({})
+        self.assert_parse(
+            {u"public-address": u"ec2-1-2-3-4.compute-1.amazonaws.com",
+             u"foo": u"bar",
+             u"configured": True})
+        self.assert_parse([])
+        self.assert_parse(["abc", "xyz", 42, True])
+        self.assert_parse(False)
+        self.assert_parse(True)
+        self.assert_parse(0.0)
+        self.assert_parse(3.14159)
+        self.assert_parse(6.02214178e23)
+        self.assert_parse(0)
+        self.assert_parse(42)
+
+        # Raises an error if no such file
+        e = self.assertRaises(
+            JujuError,
+            formatter.parse_keyvalue_pairs, ["content=@missing"])
+        self.assertEquals(
+            str(e),
+            "No such file or directory: missing (argument:content)")
+
+        # Raises an error if not of the form K=V or K=
+        e = self.assertRaises(
+            JujuError,
+            formatter.parse_keyvalue_pairs, ["foobar"])
+        self.assertEquals(
+            str(e), "Expected `option=value`. Found `foobar`")
+
+        # Raises an error if the value is invalid YAML
+        e = self.assertRaises(
+            JujuError,
+            formatter.parse_keyvalue_pairs, ["content=\xCA\FE"])
+        self.assertEquals(
+            str(e),
+            "Invalid YAML value (argument:content)")
+
+    def assert_dump_load(self, data, expected):
+        """Asserts expected formatting and roundtrip through dump/load"""
+        formatter = Format2()
+        dumped = formatter.dump({"data": data})
+        loaded = formatter.load(dumped)["data"]
+        self.assertEqual(dumped, expected)
+        self.assertEqual(data, loaded)
+        if isinstance(data, str):
+            # Verify that no promotion to unicode occurs for str values
+            self.assertInstance(loaded, str)
+
+    def test_dump_load(self):
+        """Verify JSON roundtrip semantics"""
+        self.assert_dump_load(None, "data: null")
+        self.assert_dump_load("", "data: ''")
+        self.assert_dump_load("A string", "data: A string")
+        self.assert_dump_load("High bytes: \xCA\xFE",
+                              "data: !!binary |\n    SGlnaCBieXRlczogyv4=")
+        self.assert_dump_load(u"", "data: ''")
+        self.assert_dump_load(
+            u"A unicode string (but really ascii)",
+            "data: A unicode string (but really ascii)")
+        self.assert_dump_load(u"中文", "data: \xe4\xb8\xad\xe6\x96\x87")
+        self.assert_dump_load({}, "data: {}")
+        self.assert_dump_load(
+            {u"public-address": u"ec2-1-2-3-4.compute-1.amazonaws.com",
+             u"foo": u"bar",
+             u"configured": True},
+            ("data:\n"
+             "    configured: true\n"
+             "    foo: bar\n"
+             "    public-address: ec2-1-2-3-4.compute-1.amazonaws.com"))
+        self.assert_dump_load(False, "data: false")
+        self.assert_dump_load(True, "data: true")
+        self.assert_dump_load(0.0, "data: 0.0")
+        self.assert_dump_load(3.14159, "data: 3.14159")
+        self.assert_dump_load(6.02214178e23, "data: 6.02214178e+23")
+        self.assert_dump_load(0, "data: 0")
+        self.assert_dump_load(42, "data: 42")
+
+    def test_should_delete(self):
+        """Verify only `None` values (as YAML loaded) indicate deletion"""
+        formatter = Format2()
+        self.assertFalse(formatter.should_delete("0"))
+        self.assertFalse(formatter.should_delete("something"))
+        self.assertFalse(formatter.should_delete(""))
+        self.assertFalse(formatter.should_delete("   "))
+        self.assertFalse(formatter.should_delete(0))
+        self.assertTrue(formatter.should_delete(None))

=== modified file 'juju/state/tests/test_relation.py'
--- juju/state/tests/test_relation.py	2012-05-15 19:07:47 +0000
+++ juju/state/tests/test_relation.py	2012-06-15 02:00:25 +0000
@@ -12,6 +12,7 @@
 from juju.charm.directory import CharmDirectory
 from juju.charm.tests import local_charm_id
 from juju.machine.tests.test_constraints import dummy_constraints
+from juju.charm.tests.test_metadata import test_repository_path
 from juju.charm.tests.test_repository import unbundled_repository
 from juju.lib.pick import pick_attr
 from juju.state.charm import CharmStateManager
@@ -52,6 +53,26 @@
         returnValue(service_state)
 
     @inlineCallbacks
+    def add_service_from_charm(
+                    self, service_name, charm_id=None, constraints=None,
+                    charm_dir=None, charm_name=None):
+        """Add a service from a charm.
+        """
+        if not charm_id and charm_dir is None:
+            charm_name = charm_name or service_name
+            charm_dir = CharmDirectory(os.path.join(
+                    test_repository_path, "series", charm_name))
+        if charm_id is None:
+            charm_state = yield self.charm_manager.add_charm_state(
+                    local_charm_id(charm_dir), charm_dir, "")
+        else:
+            charm_state = yield self.charm_manager.get_charm_state(
+                    charm_id)
+        service_state = yield self.service_manager.add_service_state(
+                service_name, charm_state, constraints or dummy_constraints)
+        returnValue(service_state)
+
+    @inlineCallbacks
     def add_relation(self, relation_type, *services):
         """Support older tests that don't use `RelationEndpoint`s"""
         endpoints = []
@@ -471,8 +492,9 @@
             "logging", "juju-info", "juju-info", "client", "container")
         yield self.add_service("mysql")
         yield self.add_service("logging")
-        relation_state, service_states = yield self.relation_manager.add_relation_state(
-            mysql_ep, logging_ep)
+        relation_state, service_states = \
+            yield self.relation_manager.add_relation_state(mysql_ep,
+                                                           logging_ep)
 
         # verify that the relation state instances are both marked
         # with the live scope (container). This happens even though
@@ -488,8 +510,9 @@
             "logging", "juju-info", "juju-info", "client", "container")
         yield self.add_service("mysql")
         yield self.add_service("logging")
-        relation_state, service_states = yield self.relation_manager.add_relation_state(
-            mysql_ep, logging_ep)
+        relation_state, service_states = \
+            yield self.relation_manager.add_relation_state(mysql_ep,
+                                                           logging_ep)
 
         for service_state in service_states:
             self.assertEqual(service_state.relation_scope, "container")
@@ -502,8 +525,9 @@
             "logging", "juju-info", "juju-info", "client", "global")
         yield self.add_service("mysql")
         yield self.add_service("logging")
-        relation_state, service_states = yield self.relation_manager.add_relation_state(
-            mysql_ep, logging_ep)
+        relation_state, service_states = \
+            yield self.relation_manager.add_relation_state(mysql_ep,
+                                                           logging_ep)
 
         for service_state in service_states:
             self.assertEqual(service_state.relation_scope, "container")
@@ -693,7 +717,8 @@
         id = "relation-0000000000"
         self.assertEqual(
             repr(self.service1_relation),
-            "<ServiceRelationState name:dev-connect role:prod id:%s scope:global>" % id)
+            "<ServiceRelationState name:dev-connect "
+            "role:prod id:%s scope:global>" % id)
 
     def test_property_relation_name(self):
         """