fix: align Juju naming rules with testing class rules#2570
fix: align Juju naming rules with testing class rules#2570tonyandrewmeyer wants to merge 8 commits into
Conversation
canonical#2552 made JUJU_HOOK_NAME preserve the entity-name spelling by carrying two forms of the event path (a normalised string plus an original_prefix). Simplify that to a single representation: _EventPath.name is the Python-attribute event kind (what ops calls the event, and what its handle paths use), and _EventPath.prefix is the verbatim entity name from which the Juju hook name is built. Also fix the remaining places where the simulated environment diverged from real Juju (all verified against a live Juju 3.6.23 k8s model): - Actions are dispatched as actions/<name> with no hooks/ prefix and no -action suffix; JUJU_ACTION_NAME keeps the verbatim name, and JUJU_ACTION_TAG is now set. - JUJU_RELATION_ID is '<endpoint>:<id>', not just the ID. - State.get_relations matches the endpoint verbatim instead of treating 'foo-bar' and 'foo_bar' as the same endpoint (Juju allows both spellings, as distinct endpoints). The consistency checker now compares the event's entity-name prefix to the entity name exactly, rather than a normalised startswith check (which also wrongly accepted e.g. a 'foobar' event for a 'foo' container). Checks that were unreachable now run: the workload-event 'container is in the state' error and the cannot-connect warning were nested inside the name-mismatch branch, so they only ran when the event name was already wrong; run them unconditionally, like the equivalent relation and storage checks. The 'no action instance' error message said 'workload event' and 'container'; it now refers to actions. Also add a regression test that deferred events for dashed entity names are re-emitted (the ops handle path must use the Python-attribute event kind), and remove the unused _EventPath._is_custom attribute. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Scenario accepted any string as a container, storage, relation
endpoint, or action name, so tests could pass for charms that a real
Juju deployment would reject. Validate names at construction time,
raising StateValidationError, using the same rules as Juju (verified
against juju/names and juju/charm source for both 3.x and 4.x, and
against a live Juju 3.6.23 k8s model):
- relation endpoints: lowercase alphanumeric runs separated by single
hyphens or underscores, starting with a letter (RelationSnippet).
Note that underscores ARE valid here, and Juju produces hook names
such as 'peer_under-relation-created' for them.
- storage: hyphen-separated lowercase alphanumeric, starting with a
letter, each part containing at least one letter (StorageNameSnippet).
'juju deploy' fails for an underscored storage name.
- actions: lowercase alphanumeric and hyphens, starting and ending
alphanumeric (actionNameRule). 'juju deploy' fails with 'bad action
name' for an underscored action name.
- containers: Juju passes the name verbatim to Kubernetes, so the
RFC 1123 DNS-label rule applies (no underscores, max 63 characters);
the pod is never created for an underscored container name.
Also correct the _Action docstring example, which used an action name
('do_backup') that Juju would reject.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The best-practices page said charms that standardised on underscored
action names need not change them; Juju rejects underscored action
names at deploy time ('bad action name'), so that advice was wrong
(it appears to have been copied from the config-options bullet,
where underscores are valid).
- The manage-actions how-to used ctx.on.action('do_backup'), a name
Juju would reject.
- Document on ops.CharmEvents how dashed metadata names map to the
prefixed event attributes (dashes become underscores) versus the
indexed self.on[<name>] form (verbatim).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c6e6be6 to
2c9b499
Compare
Co-authored-by: Tony Meyer <tony.meyer@gmail.com>
This file is autogenerated; changes should be made at the source. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Group each entity kind's regex and requirements-message together in _NAME_RULES, so _check_name takes only (name, kind) and call sites no longer repeat the requirements text. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The string-literal opener was dropped in a previous edit, breaking import of _consistency_checker and failing CI lint and unit tests.
There was a problem hiding this comment.
I'm a big fan of this, aligning Scenario's behaviour with Juju's here is clearly the right thing to do, and should make everyone's lives easier in the long run.
This does seem like it has the potential to be very breaky though, doesn't it? And there are two distinct types of breakage it might cause: catching real charm bugs that slipped through due to Scenario previously being incorrect; and catching testing-only reliance on Scenario's previous (incorrect) behaviour.
I haven't reviewed in depth at this point -- I'd really like to see a super-tox/Hyrum comparison for this PR before going further.
My hope, perhaps naïvely, is that everyone is using names in their tests that match the names in their charm, and so if their charm works, their tests will still work.
These ones I am ok with, although if there are a small number the practice of opening a few downstream PRs is probably good here.
This would be worse, although I would still be tempted to say that this is a bug in the charm tests.
Sure, running now. |
|
Main: 208 passed / 58 failed
I'll open downstream PRs. |
When running a pebble-ready event, the testing.State needs to contain the same testing.Container that's passed to create the (mock) event. This PR makes that change. Scenario currently has a bug meaning that this isn't checked, but that will be fixed in canonical/operator#2570
…#163) The testing.Container that is passed to `pebble-ready` events must be the same one in the testing.State. This PR fixes that (and does a bit of name collision avoidance) using the fixture already present in the tests. Scenario currently has a bug that stops this being flagged, but that will be fixed in canonical/operator#2570.
I assume those three are the only movement between passed and failed (that is, nothing new passes because of this PR). But I wonder if we need more information on any changes within the 58 that failed in both cases (e.g. any new individual tests failing). Why they're already failing is also important -- for example, if it's because we're not setting them up properly yet, then perhaps some additional manual or AI analysis of those cases would be valuable to understand if that isn't masking a possible new failure here. Since we're currently working on fleshing out super-tox (into hyrum), this PR seems like a great testing ground for this kind of analysis. |
I can dig into that. Hyrum is already better than super-tox at this, in that logging is better.
Fair, although the comparison feature has not landed yet. But I will be moving on to that now (well, after the fairly trivial private names and de-dependency changes). |
I have long been irritated by the messy way Scenario handled names, particularly with regards to underscores and dashes. #2565 has provided me with the excuse to do a full clean-up of how this is handled, making sure that everything matches real Juju properly, and avoiding inefficient (and sometimes wrong) round-tripping.
This PR aligns Scenario's simulated dispatch with how real Juju invokes hooks, and rejects entity names that Juju itself would reject, so tests can no longer pass against a charm that Juju would refuse to deploy or run.
I verified all of this against the juju/names and juju/charm sources for 3.x and 4.x, and against a live Juju 3.6.23 k8s model.
Dispatch environment and event names
#2552 made
JUJU_HOOK_NAMEpreserve the entity-name spelling by carrying two forms of the event path (a normalised string plus an original_prefix). That is now collapsed to one representation:_EventPath.nameis the Python-attribute event kind (what ops calls the event, and what its handle paths use), and_EventPath.prefixis the verbatim entity name from which the Juju hook name is built.Other places where the simulated environment diverged from real Juju:
actions/<name>with nohooks/prefix and no-actionsuffix;JUJU_ACTION_NAMEkeeps the verbatim name, andJUJU_ACTION_TAGis now set.JUJU_RELATION_IDis<endpoint>:<id>, not just the ID.State.get_relationsmatches the endpoint verbatim rather than treatingfoo-barandfoo_baras the same endpoint (Juju allows both spellings, as distinct endpoints).The consistency checker now compares the event's entity-name prefix to the entity name exactly, rather than a normalised
startswithcheck (which also wrongly accepted, for example, afoobarevent for afoocontainer).Name validation
Scenario accepted any string as a container, storage, relation endpoint, or action name. Names are now validated at construction time, raising
StateValidationError, using the same rules as Juju:RelationSnippet). Underscores are valid here, and Juju produces hook names such aspeer_under-relation-createdfor them.StorageNameSnippet).juju deployfails for an underscored storage name.actionNameRule).juju deployfails with "bad action name" for an underscored action name.Documentation fixes
manage-actionshow-to usedctx.on.action('do_backup'), a name Juju would reject.ops.CharmEventsnow documents how dashed metadata names map to the prefixed event attributes (dashes become underscores) versus the indexedself.on[<name>]form (verbatim)._Actiondocstring example useddo_backup, which Juju would reject; corrected.Drive-bys
Checks that were previously unreachable now run: the workload-event "container is in the state" error and the cannot-connect warning were nested inside the name-mismatch branch, so they only fired when the event name was already wrong; they now run unconditionally, like the equivalent relation and storage checks.
The "no action instance" error message used to say "workload event" and "container", presumably a copy and paste error; it now refers to actions.
Fixes #2565.