Layer object support for log-forwarding as a field

[kayra1]

log-targets has been added as a field to pebble as of canonical/pebble#267. However, the current implementation of ops.pebble.Layer discards this from the argument of the init. This must be handled properly so that the layer yaml file could be produced.

[benhoyt]

Thanks for the report, @kayra1. We'll aim to fix this before the next ops release (at the end of November).

We'll also look to see whether it's feasible to allow add_layer with a dict to not go via Layer.to_yaml, so that charms can use new fields before they're known by ops in future. Currently you can work around this by passing a YAML string directly to add_layer.

CC @tonyandrewmeyer

[tonyandrewmeyer]

We'll also look to see whether it's feasible to allow add_layer with a dict to not go via Layer.to_yaml, so that charms can use new fields before they're known by ops in future.

Most of what currently happens in Layer(layer).to_yaml() is loading optional fields from the dict and then dumping them back. As far as I can see, there would be two consequences of changing the Layer(layer).to_yaml() to yaml.safe_dump(layer) (other than removing the restriction to the known structure):

1. The process strips out any empty (falsy) values, e.g. Layer({"description": ""}).to_yaml() is {}\n. We could do this prior to converting the dict to yaml, but it feels more like an implementation detail of all the optional fields than something that's being done for its own sake.
2. It would be more work to add validation in the future. For example, at the moment we convert the level of a check to a LevelCheck and if it's an unknown level we just use the raw value. If we wanted that to generate a warning or error in the future, or add other similar checks, it's simpler to do in the Layer, but generally people are adding dicts/LayerDicts.

Overall, it seems a reasonable change to me. Effectively, by passing a str or dict the caller is saying they have done the work to ensure the layer is valid, and by passing a Layer they're asking ops to help with that.

[benhoyt]

@tonyandrewmeyer Thanks for the response. In our earlier discussion you raised a good point, that we want to keep ops up to date so people don't start relying on workarounds (and as you point out, so we can add further checks in future). So I think let's just add the new field(s) and not change the dict behaviour at this point. People are using dicts heavily right now, so we do want some validation on those rather than removing that guarantee.