Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't use lexical sorting for version numbers in codegen #1168

Conversation

james-garner-canonical
Copy link
Contributor

@james-garner-canonical james-garner-canonical commented Oct 18, 2024

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 (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 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 9001 comes 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.

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 james-garner-canonical force-pushed the 2024-10/fix/avoid-lexical-sorting-versions branch from 337d749 to 1392759 Compare October 18, 2024 01:49
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('.')
Copy link
Contributor

@dimaqq dimaqq Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt about using what already do for the client and server version:

from packaging import version
version.parse(server_version)

juju/client/connector.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea here to go from oldest to newest because the latter iteration overwrites the ealier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and note that schema.version is a facade version, like 7.

juju_version is the Juju schema version, like '3.1.10'.

jujubot added a commit that referenced this pull request Oct 18, 2024
…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.
Comment on lines +931 to +933
# 'latest' is special cased in load_schemas and should come last
if version_string == 'latest':
return (9000, 9000, 9000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why special-case latest?
We don't use files with the "latest" marker, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Late reply, but I special cased it in sorting because the code that loads in schemas special cases it as a permitted version. There was no need to special casing in sorting when sorting was simply lexicographic, since "latest" would sort after all the numerical strings.

@dimaqq
Copy link
Contributor

dimaqq commented Oct 22, 2024

Closing in favour of #1174

@dimaqq dimaqq closed this Oct 22, 2024
jujubot added a commit that referenced this pull request Oct 28, 2024
…ebased

#1174

Cherry-picked from #1168 and simplified.

Rationale Juju micro versions can get larger than 9, e.g. 2.9.51.

When 3.5.10 comes around, we want it to take precedence over 3.5.9 and not get wedged between 3.5.1 and 3.5.2

Keeping the current codegen mode of operation where it starts with the oldest version and whacks some internal state on encountering a latter version.

(We'll deal with that in a separate PR)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants