fix: make client_facades reflect facades in generated client code

[james-garner-canonical]

Description

client_facades in juju/client/connection.py is manually updated. It doesn't reflect the actual facade code present, which was recently generated from the current 3.1.X and 3.3.0 schemas currently checked in. Nor did it reflect the state of the generated code with the previous set of schemas (same + 3.2.X schemas), nor the state of the actually checked in code before the last regeneration of the code (which contained some orphan facades not present in the schemas).

This PR updates client_schemas and adds a test to ensure that its facade versions always match the schemas present in the generated client code. The test can be run with tox -e validate, and is run in a separate github job as it didn't seem to fit in well as either a unit or integration test. In future we could add more tests to this group to validate other aspects of the codebase related to the code generation.

QA Steps

All non-quarantined tests should continue to work.

tox -e validate

Should pass currently.

Notes & Discussion

Does the current state of client_facades make sense? One thing that stood out to me was the addition of Admin. I'm wondering if any non-client facades are being added here, since facade code is generated from the full schemas still. If that's the case, then this should be deferred till after we switch to client-only schemas.

[dimaqq]

Nice!

I think now that we know how to extract and compare, the test can be greatly simplified.

[james-garner-canonical]

Thanks for the review, @dimaqq. The integration test failures aren't the usual ones -- a lot of KeyErrors like last time I tried changing client_facades. I'll investigate further.

---

await model.deploy(... seems to be pretty reliably broken -- that's juju.Model.deploy.
It breaks on await self.deploy_types[schema].resolve(....
Model.deploy_types is defined in Model.__init__ like so:

self.deploy_types = {
    Schema.LOCAL: LocalDeployType(),
    Schema.CHARM_HUB: CharmhubDeployType(self._resolve_charm),
}

We're at least breaking with charmhub charms, given this:

# in juju.bundle
def is_local_charm(charm_url: str):
    return charm_url.startswith('.') or charm_url.startswith('local:') or os.path.isabs(charm_url)

And given these example failing lines in tests:

# test_action
app = await model.deploy('juju-qa-test')

# test_get_set_config
app = await model.deploy(
    'ubuntu',
    application_name='ubuntu',
    series='jammy',
    channel='stable',
    config={
        'hostname': 'myubuntu',
    },
    constraints={
        'arch': 'amd64',
        'mem': 256 * MB,
    },
)

# test_deploy_charmhub_charm
app = await model.deploy('ubuntu')

# test_upgrade_local_charm
app = await model.deploy('ubuntu', series='focal', revision=15, channel='latest/stable')

So let's assume we mostly just want to investigate CharmhubDeployType.resolve

That method breaks on charm_url, origin = await self.charm_resolver(url, origin, force, series, model_conf), where self.charm_resolver is just the argument to CharmhubDeployType.__init__, so that takes us to Model._resolve_charm, which breaks on this line (edited for clarity by me):

supported_series = result.get(
    'supported_series',
    result.unknown_fields['supported-series']  # KeyError!
)

So 'supported-series' isn't in the result, nor is it in result.unknown_fields.

Summary for the below: we use the Charms facade of version 7 instead of Charms version 6 -- tests passed with 6. The change is due to listing version 7 in client_facades -- previously it would have been in the code, just ignored because of not being listed.

There aren't any errors along the way, but the result is missing the expected field, so we really just need to see the result.

---

This result comes from charms_facade.ResolveCharms a few lines up, where charms_facade is essentially client.CharmsFacade.from_connection(self.connection()). self.connection() should be a connection to the relevant Juju model, so let's dig into client.CharmsFacade.from_connection.

Here it seems we end up in _client.TypeFactory.from_connection, which must be a common implementation for getting the correct generated facade. The facade we want is the name of the class minus the 'Facade' suffix. So we're trying to get the Charms facade for a given version.

What version though? We get the target version from connection.facades.get, where connection is ultimately a juju.client.connection.Connection. Its facades attribute is set in the Connection.connect constructor to an empty dict. If get returns None though, from_connection will error out, and that's not happening here, so it must get populated somehow at some point. This is presumably in Connection._build_facades, so let's assume that's been called before we get to where we are. _build_facades basically iterates over a list of facades returned from the controller, and tries to find a version in common between client_facades (or the caller's specified_facades) and the facades returned by the controller.

Now we're getting somewhere! Changing client_facades is what broke all these tests. If a match isn't found, the version isn't set, and log.warning is called -- but that's not what's happening here, because that would result in None from connection.facades.get, so it seems like we can assume that a match is found between client_facades and the facades reported by the controller.

_client.TypeFactory.from_connection then uses the version with lookup_facade, which strangely also does some fuzzy version matching. It starts at the requested version and steps downwards, seeing if any of the versions down to 1 have the facade, returning the first match. This must succeed here, since it will error if it doesn't find a match, and from_connection goes on to instantiate the returned facade class, connect it to the connection, and return it.

So we do get the Charms facade we're looking for, and that's what CharmhubDeployType.resolve uses to call ResolveCharms, which we currently have defined in _client6.py and _client7.py. Which one is being used here?

Well, client_facades used to have 'Charms': {'versions': [5, 6]} but now it has 'Charms': {'versions': [6, 7]}. Tests pass with [5, 6] even though there's no definition in _client5.py (or lower), so we must have been using 6. Since it's broken now, we can probably assume that we're now using 7.

The only difference in their implementation is the contents of the dictionary they use as the argument to self.rpc, and the only difference in those contents is the version -- 6 vs 7.

Charms.rpc resolves to facade.Type.rpc (manually defined base class for all the generated classes), which just calls self.connection.rpc -- that's the same connection that _client.TypeFactory.from_connection passed to the Charms object after instantiating it.

Heading to juju.client.connection.Connection.rpc, we can see that there's a bunch of logic and ways for errors to be raised. But we know that it doesn't raise an error! It just doesn't have a field that juju.model.Model._resolve_charm expects, specifically 'supported-series'. So really we just want to see what's in that result ... (which should probably have been where we first looked anyway ...)

[dimaqq]

It seems that "supported-series" field was removed after Juju 3.2.3
What does the library need this field for, really?

[dimaqq]

Ahh I see your latest commit, it makes sense that "bases" replaced "series".
I'm not sure if the values are the same though.

See:
https://discourse.charmhub.io/t/transition-from-series-to-base-in-juju-4-0/14127
https://juju.is/docs/juju/base

[james-garner-canonical]

Ahh I see your latest commit, it makes sense that "bases" replaced "series". I'm not sure if the values are the same though.

See:
https://discourse.charmhub.io/t/transition-from-series-to-base-in-juju-4-0/14127
https://juju.is/docs/juju/base

Yeah I didn't think it would work, but figured it was worth seeing what would happen. Thanks for finding that documentation.

[dimaqq]

How about this:

special-case that one specific facade, so that the rest of the PR can be refactored, rebased on top of #1155 and merged.

open an issue to resolve series vs bases. I think a short discussion with someone from Juju is on order, depending on e.g. what range of Juju versions the library should support.

[dimaqq]

#1156

[james-garner-canonical]

I believe integration tests are passing now, they're just rerunning from the latest minor push. Please take a look, @dimaqq. I think I need to document excluded_facades somewhere, but not sure where would be best.

[james-garner-canonical]

/merge