feat: add support for v2 fields in CharmMeta #1106
Conversation
|
@benhoyt this needs the new classes added to |
Thanks! Just needed a santy check 😄. You can go over in detail when I finish it off 😄.
The one linked in the description: https://juju.is/docs/sdk/metadata-yaml - the Discourse post linked from the issue says that it should be used rather than the Discourse post. |
| raise AttributeError( | ||
| f"{self.__class__.__name__} has no such attribute: {name}!" | ||
| ) | ||
| @property |
There was a problem hiding this comment.
This is a backwards-incompatible change in that you can no longer assign to .location, but it seems like that would surely always be a bug anyway, since it wouldn't actually change the location, just make the metadata incorrect.
There was a problem hiding this comment.
Excellent, thanks for this fix.
|
FYI, I did a basic sanity check across the ~150 charms I have locally, and loading the metadata doesn't crash: import pathlib
import ops
root = pathlib.Path(...)
for charm in root.iterdir():
meta = charm / 'metadata.yaml'
if meta.exists():
assert ops.CharmMeta.from_yaml(meta.open()).name |
ops/charm.py
Outdated
| self.min_juju_version = raw_.get('min-juju-version') | ||
| if self.min_juju_version: | ||
| # Add in an implied 'assumes'. | ||
| self.assumes.features.append(f'juju >= {self.min_juju_version}') |
There was a problem hiding this comment.
Are we sure we want to add this in? Why? It would seem better to me to have this reflect the metadata as is, rather than try to be smart about things.
There was a problem hiding this comment.
I definitely agree that the code I had earlier that tried to be smart in the other direction (populating min_juju_version from assumes) was a bad idea (on the positive side, it helped me really dig into how assumes works, so I could be more sure I had the class correct 😄).
This direction (populating assumes from min_juju_version) is vastly simpler, just these 2-3 lines, and more obviously correct.
My thinking was that if someone is using this in code, and they want to support both versions of metadata (e.g. in a lib where you don't have as much control over the metadata), they have to do something like:
min_version = meta.min_juju_version or find_minimum_juju_version(meta.assumes)instead of just find_minimum_juju_version(meta.assumes).
However, writing this out I think I have rubber-ducked my way into agreeing that it's too small to be worthwhile 😄.
| raise AttributeError( | ||
| f"{self.__class__.__name__} has no such attribute: {name}!" | ||
| ) | ||
| @property |
There was a problem hiding this comment.
Excellent, thanks for this fix.
There was a problem hiding this comment.
The assumes stuff looks good (consistent with how juju parses it).
juju reads bases from manifest.yaml and what's here looks consistent with that.
Can we defer adding Devices until we are sure it's actually used somewhere?
Otherwise looks ok to me except for the properties bit.
Extends the
CharmMetaand related objects to handle all fields in the current specification, both loading frommetadata.yamland fromcharmcraft.yaml(which is likely to happen when running unit tests).New fields:
links:websitessourcesissuesdocumentation(this is "docs" in metadata.yaml, but "documentation" in charmcraft.yaml, and is a single string unlike the above, so "docs" seems misleading)assumesmetadata.yamlallows arbitrary fields, so leaving in fields that are not in the current specification (e.g. "tags", "series") seems ok and provides the most backwards compatibility.Also fleshes out
ContainerMetaand addspropertiestoStorageMeta.Fixes #498.