Repository: canonical/operator

Issue:
Can't send environment to pebble
I'm working on converting a charm to use pebble (as a proof of concept) and am running into problems trying to send it an `environment` configuration.

The pebble docs specify:

```
        environment:
            - VAR1: val1
            - VAR2: val2
            - VAR3: val3
```

So I tried passing a list of dictionaries to the environment config, but I got:
```
unit-gunicorn-0: 10:04:56 INFO unit.gunicorn/0.juju-log About to dump yaml config <<EOM
description: gunicorn layer
services:
  gunicorn:
    command: /srv/gunicorn/run
    default: start
    environment:
    - FAVOURITEFOOD: burgers
    - FAVOURITEDRINK: ale
    override: replace
    summary: gunicorn service
summary: gunicorn layer

EOM
unit-gunicorn-0: 10:04:56 ERROR unit.gunicorn/0.juju-log Uncaught exception while in charm code:
Traceback (most recent call last):
  File "./src/charm.py", line 438, in <module>
    main(GunicornK8sCharm, use_juju_for_storage=True)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/main.py", line 406, in main
    _emit_charm_event(charm, dispatcher.event_name)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/main.py", line 140, in _emit_charm_event
    event_to_emit.emit(*args, **kwargs)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/framework.py", line 278, in emit
    framework._emit(event)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/framework.py", line 722, in _emit
    self._reemit(event_path)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/framework.py", line 767, in _reemit
    custom_handler(event)
  File "./src/charm.py", line 137, in _on_gunicorn_workload_ready
    container.add_layer("gunicorn", pebble_config)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/model.py", line 1065, in add_layer
    self._pebble.add_layer(label, layer, combine=combine)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/pebble.py", line 668, in add_layer
    layer_yaml = Layer(layer).to_yaml()
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/pebble.py", line 427, in __init__
    self.services = {name: Service(name, service)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/pebble.py", line 427, in <dictcomp>
    self.services = {name: Service(name, service)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/pebble.py", line 463, in __init__
    self.environment = dict(raw.get('environment') or {})
ValueError: dictionary update sequence element #0 has length 1; 2 is required
```
Looking at the code for the operator framework `self.environment = dict(raw.get('environment') or {})` so I tried just passing in a dictionary, but I got the following:
```
unit-gunicorn-0: 10:18:33 ERROR juju.worker.uniter pebble poll failed: hook failed
unit-gunicorn-0: 10:18:37 INFO unit.gunicorn/0.juju-log About to dump yaml config <<EOM
description: gunicorn layer
services:
  gunicorn:
    command: /srv/gunicorn/run
    default: start
    environment:
      FAVOURITEDRINK: ale
      FAVOURITEFOOD: burgers
    override: replace
    summary: gunicorn service
summary: gunicorn layer

EOM
unit-gunicorn-0: 10:18:37 ERROR unit.gunicorn/0.juju-log Uncaught exception while in charm code:
Traceback (most recent call last):
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/pebble.py", line 531, in _request
    response = self.opener.open(request, timeout=self.timeout)
  File "/usr/lib/python3.8/urllib/request.py", line 531, in open
    response = meth(req, response)
  File "/usr/lib/python3.8/urllib/request.py", line 640, in http_response
    response = self.parent.error(
  File "/usr/lib/python3.8/urllib/request.py", line 569, in error
    return self._call_chain(*args)
  File "/usr/lib/python3.8/urllib/request.py", line 502, in _call_chain
    result = func(*args)
  File "/usr/lib/python3.8/urllib/request.py", line 649, in http_error_default
    raise HTTPError(req.full_url, code, msg, hdrs, fp)
urllib.error.HTTPError: HTTP Error 400: Bad Request

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "./src/charm.py", line 450, in <module>
    main(GunicornK8sCharm, use_juju_for_storage=True)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/main.py", line 406, in main
    _emit_charm_event(charm, dispatcher.event_name)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/main.py", line 140, in _emit_charm_event
    event_to_emit.emit(*args, **kwargs)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/framework.py", line 278, in emit
    framework._emit(event)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/framework.py", line 722, in _emit
    self._reemit(event_path)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/framework.py", line 767, in _reemit
    custom_handler(event)
  File "./src/charm.py", line 150, in _on_gunicorn_workload_ready
    container.add_layer("gunicorn", pebble_config)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/model.py", line 1065, in add_layer
    self._pebble.add_layer(label, layer, combine=combine)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/pebble.py", line 682, in add_layer
    self._request('POST', '/v1/layers', body=body)
  File "/var/lib/juju/agents/unit-gunicorn-0/charm/venv/ops/pebble.py", line 542, in _request
    raise APIError(body, code, status, message)
ops.pebble.APIError: cannot parse layer YAML: cannot parse layer "gunicorn": yaml: unmarshal errors:
  line 7: cannot unmarshal !!map into []plan.StringVariable
unit-gunicorn-0: 10:18:38 ERROR juju.worker.uniter.operation hook "gunicorn-workload-ready" (via hook dispatching script: dispatch) failed: exit status 1
```

Discussion:
benhoyt: > I'm no expert on idiomatic python but the kv as a tuple seems appropriate.

Yeah, I think so. For example, the `dict()` function accepts that forma, and `dict.items()` yields key-value tuples:

```python
>>> d = dict([('a', 1), ('b', 2)])
>>> list(d.items())
[('a', 1), ('b', 2)]
```

Patch:
diff --git a/ops/pebble.py b/ops/pebble.py
index b95c969..2f78aa3 100644
--- a/ops/pebble.py
+++ b/ops/pebble.py
@@ -447,7 +447,12 @@ class Layer:
 
 
 class Service:
-    """Represents a service description in a Pebble configuration layer."""
+    """Represents a service description in a Pebble configuration layer.
+
+    The "environment" attribute is parsed as a list of (key, value) tuples,
+    because that seems most natural for ordered keys and values in Python.
+    In the YAML, however, it's a list of 1-item {key: value} objects.
+    """
 
     def __init__(self, name: str, raw: Dict = None):
         self.name = name
@@ -460,7 +465,18 @@ class Service:
         self.after = list(raw.get('after', []))
         self.before = list(raw.get('before', []))
         self.requires = list(raw.get('requires', []))
-        self.environment = dict(raw.get('environment') or {})
+        self.environment = self._dicts_to_tuples(raw.get('environment', []))
+
+    @staticmethod
+    def _dicts_to_tuples(dicts):
+        """Convert list of 1-item {k: v} dicts to list of (k, v) tuples."""
+        tuples = []
+        for d in dicts:
+            if len(d) != 1:
+                raise ValueError('expected 1-item dict, got {!r}'.format(d))
+            kv = list(d.items())[0]
+            tuples.append(kv)
+        return tuples
 
     def to_dict(self) -> Dict:
         """Convert this service object to its dict representation."""
@@ -473,7 +489,7 @@ class Service:
             ('after', self.after),
             ('before', self.before),
             ('requires', self.requires),
-            ('environment', self.environment),
+            ('environment', [{k: v} for k, v in self.environment]),
         ]
         return {name: value for name, value in fields if value}
 


Test patch:
diff --git a/test/pebble_cli.py b/test/pebble_cli.py
index 706ce51..ae3e198 100644
--- a/test/pebble_cli.py
+++ b/test/pebble_cli.py
@@ -109,7 +109,7 @@ def main():
             result = client.get_changes(select=pebble.ChangeState(args.select),
                                         service=args.service)
         elif args.command == 'plan':
-            result = client.get_plan().raw_yaml
+            result = client.get_plan().to_yaml()
         elif args.command == 'start':
             result = client.start_services(args.service)
         elif args.command == 'stop':
diff --git a/test/test_pebble.py b/test/test_pebble.py
index 6539381..7b63bff 100644
--- a/test/test_pebble.py
+++ b/test/test_pebble.py
@@ -418,6 +418,9 @@ class TestLayer(unittest.TestCase):
 services:
   bar:
     command: echo bar
+    environment:
+    - ENV1: value1
+    - ENV2: value2
     summary: Bar
   foo:
     command: echo foo
@@ -433,6 +436,8 @@ summary: Sum Mary
         self.assertEqual(s.services['bar'].name, 'bar')
         self.assertEqual(s.services['bar'].summary, 'Bar')
         self.assertEqual(s.services['bar'].command, 'echo bar')
+        self.assertEqual(s.services['bar'].environment,
+                         [('ENV1', 'value1'), ('ENV2', 'value2')])
 
         self.assertEqual(s.to_yaml(), yaml)
         self.assertEqual(str(s), yaml)
@@ -449,7 +454,7 @@ class TestService(unittest.TestCase):
         self.assertEqual(service.after, [])
         self.assertEqual(service.before, [])
         self.assertEqual(service.requires, [])
-        self.assertEqual(service.environment, {})
+        self.assertEqual(service.environment, [])
         self.assertEqual(service.to_dict(), {})
 
     def test_name_only(self):
@@ -469,7 +474,7 @@ class TestService(unittest.TestCase):
             'after': ['a1', 'a2'],
             'before': ['b1', 'b2'],
             'requires': ['r1', 'r2'],
-            'environment': {'k1': 'v1', 'k2': 'v2'},
+            'environment': [{'k1': 'v1'}, {'k2': 'v2'}],
         }
         s = pebble.Service('Name 2', d)
         self.assertEqual(s.name, 'Name 2')
@@ -480,7 +485,7 @@ class TestService(unittest.TestCase):
         self.assertEqual(s.after, ['a1', 'a2'])
         self.assertEqual(s.before, ['b1', 'b2'])
         self.assertEqual(s.requires, ['r1', 'r2'])
-        self.assertEqual(s.environment, {'k1': 'v1', 'k2': 'v2'})
+        self.assertEqual(s.environment, [('k1', 'v1'), ('k2', 'v2')])
 
         self.assertEqual(s.to_dict(), d)
 
@@ -488,15 +493,15 @@ class TestService(unittest.TestCase):
         s.after.append('a3')
         s.before.append('b3')
         s.requires.append('r3')
-        s.environment['k3'] = 'v3'
+        s.environment.append(('k3', 'v3'))
         self.assertEqual(s.after, ['a1', 'a2', 'a3'])
         self.assertEqual(s.before, ['b1', 'b2', 'b3'])
         self.assertEqual(s.requires, ['r1', 'r2', 'r3'])
-        self.assertEqual(s.environment, {'k1': 'v1', 'k2': 'v2', 'k3': 'v3'})
+        self.assertEqual(s.environment, [('k1', 'v1'), ('k2', 'v2'), ('k3', 'v3')])
         self.assertEqual(d['after'], ['a1', 'a2'])
         self.assertEqual(d['before'], ['b1', 'b2'])
         self.assertEqual(d['requires'], ['r1', 'r2'])
-        self.assertEqual(d['environment'], {'k1': 'v1', 'k2': 'v2'})
+        self.assertEqual(d['environment'], [{'k1': 'v1'}, {'k2': 'v2'}])
 
 
 class MockClient(pebble.Client):