Discrepancy between template and tutorial snippet

[bittner]

Bug Description

The _pebble_layer() member in the init-simple template seems to be missing code that wraps the Python dictionary into a Pebble Layer object. That code, on the other hand, is included in the Juju Tutorial for Kubernetes.

It looks like wrapping the dictionary in a Layer object has been forgotten (to update?) in the template code.

EDIT: Note that I'm a newbie and that the Juju SDK docs mention that it's possible to add a layer both with a plain YAML string or Python dict, and with a Layer object. Hence, the question is, is there a preferred way to do it.

To Reproduce

1. Go to GitHub > canonical/charmcraft > charmcraft/templates/init-simple/src/charm.py.j2.
2. Compare with code snippet suggested in Juju SDK docs > Tutorials > Kubernetes > Create minimal charm.

Related

• Juju SDK docs > How-to Guides > Interact with Pebble > Add a configuration layer

[syncronize-issues-to-jira[bot]]

Thank you for reporting us your feedback!

The internal ticket has been created: https://warthogs.atlassian.net/browse/CRAFT-3183.

This message was autogenerated

[lengau]

Thanks for the report! For the question about the preferred ways to add a layer I'd like to get @tonyandrewmeyer or @benhoyt to chime in. I would guess that the Layer object is preferred since it'd be the strictest form, but the ops source indicates that it's using the strting in the end anyway.

I agree about the templates issue - I'll need to get multiple stakeholders involved to discuss the best way to do that in the templates though.

[benhoyt]

Probably if I were building the dict inline right above the add_layer call, I'd just use a plain dict. However, given there's a property called _pebble_layer I think the K8s tutorial is probably better -- it's return type would be a "proper" pebble.Layer object. So my take would be to change the template.

But I'd love to hear if @tonyandrewmeyer has good thoughts either way.

[tonyandrewmeyer]

Yeah, given that add_layer will immediately convert a LayerDict to a Layer it doesn't really make much difference.

I agree with both of @benhoyt's points. In addition:

If I was writing from scratch, I would generally use a Layer, because I'd start writing:

def _pebble_layer(self):
    return ops.pebble.Layer({"

And at that point I'd get autocomplete for the fields. However, if you do this:

def _pebble_layer(self) -> ops.pebble.LayerDict:
    return {"

Then you get the autocomplete too, so it's not a strong advantage (I generally prefer the implicit return types).

I prefer it for testing the layer property, too, e.g. this:

assert layer.services["x"].command == 

Rather than

assert layer["services"]["x"]["command"] ==

And also when testing events that need the layer to be present for some reason, e.g.:

layer = ops.pebble.Layer({...})
container = Container(layers={layer})
...
assert layer in state.get_container(container.name).layers

Although this is also not much of an issue since Layers can be compared to LayerDicts, but it feels cleaner to be comparing the same types.

If I didn't know how it was implemented, I think it would also 'feel' stronger - I'd imagine that there could be additional value checking when creating the object and wouldn't know that the object creation happens anyway.

I would definitely not pass a YAML string, unless I was sourcing that from somewhere else (like a file). I think that's in the how-to guide because it's directly stating what's in the reference docs rather than because it's an equally common choice.