Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5240)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 10 months ago by jimbaker
Modified:
11 years, 10 months ago
Reviewers:
mp+108831, hazmat, niemeyer, fwereade
Visibility:
Public.

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 <clint@ubuntu.com> description: | WordPress is a full featured web blogging tool: - Instant publishing (no rebuilding) - Comment pingback support with spam protection - Non-crufty URLs - Themable - Plugin support requires: db: interface: mysql provides: website: interface: http format: 2 format: 1 ========= When using format: 1 (this format is implied when the format key is undefined in ``metadata.yaml``), Juju for this charm maintains existing format support for specifying configuration and relation settings and for outputting relation settings when using the smart format for ``relation-get``. When using ASCII strings to define scalar values, this is not an issue for working with relation settings. For config settings, because everything is a string, it's not possible to actually set boolean config settings (regardless of the capitalization): $ juju set wordpress good=True 2012-06-05 14:44:35,893 INFO Connecting to environment... 2012-06-05 14:44:38,890 INFO Connected to environment. Invalid value for good: 'True' 2012-06-05 14:44:39,108 ERROR Invalid value for good: 'True' When a map is output (``relation-get -``), smart format continues to use the ``__str__`` formatting, in this case of the internally used Python dict. In this, we also see the impact of how parsing all values as strings (in relation-set/set), then using JSON for encoding transport converts all strings to Unicode. This results in output like the following: {u"public-address": u"ec2-1-2-3-4.compute-1.amazonaws.com", u"configured": u"True", u"foo": u"bar"} Per the discussion on the mailing list, this exposes Python implementation details and needs to be fixed, especially in light of the Go port. format: 2 ========= When the format is set to 2, relation (``relation-set``) and config (``juju set``) settings are parsed as YAML. When using ``relation-get`` with smart format, output is in YAML, using the same YAML formatting options as ``juju get`` for consistency/human readibility. Output for v2 format is as follows for scalars, each on a separate line: true # Input was true (or True or TRUE, YAML is especially tolerant of its input) false A string Maps are output like so. A bash script likely could even readily written to parse this output, without the use of a helper: configured: true foo: bar public-address: ec2-1-2-3-4.compute-1.amazonaws.com It is possible for charms of different charm format to be deployed in an environment. Besides unit testing, this scenario has been extensively tested when deployed. New environment variable: CHARM_FORMAT ====================================== For relation-set/relation-get, the new environment variable ``CHARM_FORMAT``, as set for the relation hook context, controls parsing and output. This can also be used in debug hooks to see the impact of choosing one format versus the other. Implementation notes ==================== Overall code changes are small, with most changes seen in tests. In addition, tests were added to describe the current observed format: 1 behavior. Two parallel code paths were implemented in impacted code. If it's desired to modify the behavior of format: 2, this can be readily done on that code path. yaml.safe_load and yaml.safe_dump are used to ensure that Python types do not leak and to avoid introducing a security hole. The use of YAML also allows for consistent support of Unicode strings (not using ASCII codepoints) as well as binary data (as introduced via the !!binary tag). Unlike the format: 1 implementation, the consistent use of YAML for encoding/decoding ensures that there is not an unexpected encoding issue that's encountered, such as seen in bug #901495. https://code.launchpad.net/~jimbaker/juju/charm-format-2/+merge/108831 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 39

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

Total comments: 6

Patch Set 3 : Adds format: 2 support to juju, while maintaining backwards compatibility for old charms. #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+1102 lines, -163 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M juju/charm/config.py View 1 2 2 chunks +6 lines, -1 line 2 comments Download
M juju/charm/metadata.py View 1 2 5 chunks +16 lines, -6 lines 2 comments Download
A juju/charm/tests/repository/series/mysql-format-v2/config.yaml View 1 chunk +17 lines, -0 lines 0 comments Download
A juju/charm/tests/repository/series/mysql-format-v2/metadata.yaml View 1 chunk +6 lines, -0 lines 0 comments Download
A juju/charm/tests/repository/series/mysql-format-v2/revision View 1 chunk +1 line, -0 lines 0 comments Download
A juju/charm/tests/repository/series/mysql/config.yaml View 1 chunk +17 lines, -0 lines 0 comments Download
M juju/charm/tests/repository/series/mysql/metadata.yaml View 1 chunk +1 line, -0 lines 0 comments Download
M juju/charm/tests/test_config.py View 1 3 chunks +3 lines, -3 lines 0 comments Download
M juju/charm/tests/test_metadata.py View 2 chunks +24 lines, -0 lines 0 comments Download
M juju/control/config_get.py View 1 2 2 chunks +34 lines, -35 lines 0 comments Download
M juju/control/config_set.py View 1 2 4 chunks +5 lines, -3 lines 0 comments Download
M juju/control/tests/test_config_set.py View 1 chunk +134 lines, -0 lines 0 comments Download
M juju/hooks/cli.py View 1 2 5 chunks +6 lines, -29 lines 0 comments Download
M juju/hooks/invoker.py View 1 3 chunks +10 lines, -0 lines 0 comments Download
M juju/hooks/protocol.py View 1 2 9 chunks +32 lines, -18 lines 2 comments Download
M juju/hooks/tests/test_cli.py View 1 2 3 chunks +98 lines, -57 lines 0 comments Download
M juju/hooks/tests/test_communications.py View 1 4 chunks +9 lines, -2 lines 0 comments Download
M juju/hooks/tests/test_invoker.py View 1 5 chunks +175 lines, -2 lines 2 comments Download
A juju/lib/format.py View 1 2 1 chunk +144 lines, -0 lines 2 comments Download
A juju/lib/tests/test_format.py View 1 2 1 chunk +330 lines, -0 lines 0 comments Download
M juju/state/tests/test_relation.py View 6 chunks +32 lines, -7 lines 0 comments Download

Messages

Total messages: 17
jimbaker
Please take a look.
11 years, 10 months ago (2012-06-05 21:10:56 UTC) #1
niemeyer
I haven't reviewed the code, but the description of the change looks good. One detail ...
11 years, 10 months ago (2012-06-06 01:29:38 UTC) #2
niemeyer
On further thinking, if the environment variable makes the change simpler, we may as well ...
11 years, 10 months ago (2012-06-06 02:18:58 UTC) #3
jimbaker
On 2012/06/06 02:18:58, niemeyer wrote: > On further thinking, if the environment variable makes the ...
11 years, 10 months ago (2012-06-06 02:57:56 UTC) #4
fwereade
OK, this is a bit of a monster review... sorry :(. I have some suggestions ...
11 years, 10 months ago (2012-06-06 13:34:40 UTC) #5
fwereade
Another note: relation-ids and relation-list don't generate yaml; this is odd, given that we favour ...
11 years, 10 months ago (2012-06-06 14:47:29 UTC) #6
fwereade
Comments re: internal serialization format withdrawn; they're necessary to preserve charm format 1 bugginess. Would ...
11 years, 10 months ago (2012-06-06 15:30:45 UTC) #7
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, ...
11 years, 10 months ago (2012-06-07 04:40:43 UTC) #8
jimbaker
Comments addressed in the latest version of the proposal.
11 years, 10 months ago (2012-06-07 04:41:22 UTC) #9
jimbaker
Yes, it's now there.
11 years, 10 months ago (2012-06-07 04:41:58 UTC) #10
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 ...
11 years, 10 months ago (2012-06-12 21:18:08 UTC) #11
jimbaker
Please take a look.
11 years, 10 months ago (2012-06-15 01:56:45 UTC) #12
jimbaker
Please take a look at the latest proposed version of this branch, this should solve ...
11 years, 10 months ago (2012-06-15 02:10:39 UTC) #13
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 ...
11 years, 10 months ago (2012-06-18 22:46:41 UTC) #14
hazmat
i'd still like to get rid of the env var, but i can live with ...
11 years, 10 months ago (2012-06-19 20:01:09 UTC) #15
jimbaker
On 2012/06/19 20:01:09, hazmat wrote: > i'd still like to get rid of the env ...
11 years, 10 months ago (2012-06-19 22:06:21 UTC) #16
jimbaker
11 years, 10 months ago (2012-06-19 22:07:08 UTC) #17
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#newc...
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#newc...
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_invok...
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/6308044/diff/11004/juju/hooks/tests/test_invok...
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!
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b