fix: don't use lexical sorting for version numbers in codegen #1168
Conversation
Previously, python-libjuju iterated over schemas keyed by their version string (e.g. '3.1.9') using lexical sorting. For a given facade version, a definition in a higher versioned schema was intended to overwrite any prior definition saved (see generate_facades function in facade.py). With lexical sorting, '3.1.10' would be sorted in between '3.1.1' and '3.1.2', which would not lead to the desired behaviour. This commit fixes this problem by using a tuple of integers as the sorting key. A special case is requried for the version string 'latest', and we use (9000, 9000, 9000).
james-garner-canonical
force-pushed
the
2024-10/fix/avoid-lexical-sorting-versions
branch
from
337d749
to
1392759
Compare
| if version_string == 'latest': | ||
| return (9000, 9000, 9000) | ||
| # raise ValueError if string isn't in the format A.B.C | ||
| major, minor, micro = version_string.split('.') |
There was a problem hiding this comment.
wdyt about using what already do for the client and server version:
from packaging import version
version.parse(server_version)
juju/client/connector.py
There was a problem hiding this comment.
Sounds like a good idea, if you want to take care of it to push this over the finish line that would be cool, otherwise I'll hopefully get to it some time during the sprint
| captures = defaultdict(codegen.Capture) | ||
|
|
||
| # Build the Facade classes | ||
| for juju_version in sorted(schemas.keys()): | ||
| for juju_version in sorted(schemas.keys(), key=sortable_schema_version): |
There was a problem hiding this comment.
Is the idea here to go from oldest to newest because the latter iteration overwrites the ealier?
There was a problem hiding this comment.
Email reply isn't as cool as I was hoping. But yeah, I think it does overwrite.
The relevant line is:
captures[schema.version].clear(cls_name)
With this PR I'm just slavishly trying to keep the original intent working. But maybe this being technically broken is an opportunity for us to make some bigger changes?
Certainly in terms of implementation it would be nice to get rid of all the inheriting from dict, and have those classes just do operations on an internal dict instead -- at most implement the specific dict methods that are expected to be used and forward them to the internal dict rather than inheriting every dict method and overriding some of them ...
There was a problem hiding this comment.
Oh and note that schema.version is a facade version, like 7.
juju_version is the Juju schema version, like '3.1.10'.
…ema-release-process #1169 #### Description We want to ensure that `python-libjuju` supports the latest Juju schemas, and that it's clear to `python-libjuju` maintainers and users what versions are supported. This PR adds documentation on how to do this (`docs/CONTRIBUTING.md`), and follows this process for the existing schemas, updating the `3.1` series (`juju/client/SCHEMAS.md` and `juju/client/schemas-juju-*.json`) #### QA Steps No changes to generated code are introduced in this PR. `make client` should not produce any changes to the repo state. Test should all pass. #### Notes & Discussion Replaces: - #1166 Implicitly depends on: - #1168 Since we add a schema for `3.1.10` here.
Description
Previously, python-libjuju iterated over schemas keyed by their version string (e.g.
'3.1.9') using lexical sorting. For a given facade version, a definition in a higher versioned schema was intended to overwrite any prior definition saved (seegenerate_facadesfunction infacade.py). With lexical sorting,'3.1.10'would be sorted in between'3.1.1'and'3.1.2', which would not lead to the desired behaviour. This commit fixes this problem by using a tuple of integers as the sorting key. A special case is required for the version string'latest', and we use(9000, 9000, 9000).QA Steps
Codegen doesn't change with current schemas.
Notes & Discussion
When
Juju 9001comes out (well, any Juju version >9000.9000.9000), we'll need to update the special case for'latest'. Actually we can probably just remove the'latest'special case if we ever tidy up codegen, as it's not used currently, and is probably just intended as a convenience for testing and development, where it still doesn't even really seem necessary.